Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement sei/cli using port{EXIT,ENTER}_CRITICAL #835

Closed
wants to merge 3 commits into from

Conversation

rtwfroody
Copy link

This fixes the library I use to read temperature/humidity from DHT22.

One problem is that the FreeRTOS functions work when nested, while I believe the Arduino sei() enables interrupts regardless of how many times cli() was invoked. Another problem is that these FreeRTOS functions don't work when called from interrupt handlers. (There are others for that.)

These caveats aside, this does seem to work in my thermostat project.

This is probably not ready to merge, but might be a good starting point for a more complete solution. It helps with issue #832.

This fixes the library I use to read temperature/humidity from DHT22.
@@ -25,6 +25,8 @@
#define HWTIMER_LOCK() portENTER_CRITICAL(timer->lock)
#define HWTIMER_UNLOCK() portEXIT_CRITICAL(timer->lock)

portMUX_TYPE arduino_mux = portMUX_INITIALIZER_UNLOCKED;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first mayb e move those to esp32-hal.h and esp32-hal-misc.c ? should not be in the timers ;)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps also rename arduino_mux to interrupt_lock or interrupt_mux ?

cli/sei are defined to just disable/enable interrupts, while the
FreeRTOS port{ENTER,EXIT}CRITICAL functions must always match, and can
be nested. Deal with this discrepancy by tracking whether interrupts
were already disabled, and only call the FreeRTOS function if something
will change.
@gtalusan
Copy link
Contributor

I'm a bit confused by this.... wouldn't you want to actually disable interrupts instead of locking on a mutex? I see there are calls for xt_ints_off and xt_ints_on that let you directly mask the interrupt. There's also ets_intr_lock and ets_intr_unlock that big hammer all interrupts.

@me-no-dev
Copy link
Member

http://esp-idf.readthedocs.io/en/latest/api-guides/freertos-smp.html#critical-sections-disabling-interrupts

Question is: What do you want to achieve by cli/sei? Protect memory? Make sure nothing interrupts particular code?

@timsifive
Copy link

@gtalusan Actually disabling interrupts would be better, although then you might lose things like the watchdog. I suppose that's just inevitable if you're trying to implement Arduino's simple view of the world in a more complex system.

@me-no-dev The DHT sensor code needs to disable interrupts so it can count how long its output signal
is low/high. The sensor generates a sequence of high pulses on its signal line without a clock, and the reader needs to compare the duration of the high pulses to a reference pulse. So the code disables interrupts, and polls the signal in a tight loop, counting the number of loop iterations that the signal is high. Obviously interrupts interfere with that.

Strangely, now that I've migrated my project to use the Arduino layer as a component in esp-idf, reading the temperature seems to work often enough without applying this patch at all. It works about 93% of the time without this patch. With the patch it works 100% of the time.

@stickbreaker
Copy link
Contributor

@rtwfroody I did a similar modification to the OneWire library #951 . It seems to work just fine.

Chuck.

@luc-github
Copy link
Contributor

Sorry to comment, and forgive me if I am wrong

cli() is same as noInterrupts() & sei() is same as interrupts() so should be according

#define portENTER_CRITICAL(mux) vTaskEnterCritical(mux, __FUNCTION__, __LINE__)
#define portEXIT_CRITICAL(mux) vTaskExitCritical(mux, __FUNCTION__, __LINE__)
#define portENTER_CRITICAL_ISR(mux) vTaskEnterCritical(mux, __FUNCTION__, __LINE__)
#define portEXIT_CRITICAL_ISR(mux) vTaskExitCritical(mux, __FUNCTION__, __LINE__)
#else
void vTaskExitCritical( portMUX_TYPE *mux );
void vTaskEnterCritical( portMUX_TYPE *mux );
void vPortCPUAcquireMutex(portMUX_TYPE *mux);
/** @brief Acquire a portmux spinlock with a timeout
*
* @param mux Pointer to portmux to acquire.
* @param timeout_cycles Timeout to spin, in CPU cycles. Pass portMUX_NO_TIMEOUT to wait forever,
* portMUX_TRY_LOCK to try a single time to acquire the lock.
*
* @return true if mutex is successfully acquired, false on timeout.
*/
bool vPortCPUAcquireMutexTimeout(portMUX_TYPE *mux, int timeout_cycles);
void vPortCPUReleaseMutex(portMUX_TYPE *mux);
#define portENTER_CRITICAL(mux) vTaskEnterCritical(mux)
#define portEXIT_CRITICAL(mux) vTaskExitCritical(mux)
#define portENTER_CRITICAL_ISR(mux) vTaskEnterCritical(mux)
#define portEXIT_CRITICAL_ISR(mux) vTaskExitCritical(mux)
#endif
// Critical section management. NW-TODO: replace XTOS_SET_INTLEVEL with more efficient version, if any?
// These cannot be nested. They should be used with a lot of care and cannot be called from interrupt level.
//
// Only applies to one CPU. See notes above & below for reasons not to use these.
#define portDISABLE_INTERRUPTS() do { XTOS_SET_INTLEVEL(XCHAL_EXCM_LEVEL); portbenchmarkINTERRUPT_DISABLE(); } while (0)
#define portENABLE_INTERRUPTS() do { portbenchmarkINTERRUPT_RESTORE(0); XTOS_SET_INTLEVEL(0); } while (0)

portDISABLE_INTERRUPTS() and portENABLE_INTERRUPTS() per my understanding

portENTER_CRITICAL(&interrupt_mux); and portEXIT_CRITICAL(&interrupt_mux); which call vTaskEnterCritical(mux) and vTaskExitCritical(mux) have different purpose , no ?

@me-no-dev
Copy link
Member

@luc-github you are correct :) closing this. Feel free to add one with the proper defines

@me-no-dev me-no-dev closed this Mar 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants