Nested OC_LOCK_OD causing permanently disabled interrupts.

[somlioy]

Hi

After the update in CanopenNode where OD is always locked on a SDO call (Always call OD lock macros on SDO operations), my code broke down; the interrupts isnt enabled again.

This change caused nested OC_LOCK_OD to happen and since there is only one variable for storing the primask in the CO_CANmodule_t struct, the interrupts will never be enabled again.

The result was that the code stopped working on a HAL_Delay (infinite while loop) in anothert part of the application since OC_LOCK/__disable_irq apparantly also disable systicks.

I noticed this during a eeprom save (0x1010) because I had OC_LOCK_OD/UNLOCK on the OD-entry to be saved in the "storeEeprom"-function.

Is there any better way to implement this?
Ie. not disabling global interrupts, only the timer in charge of the interrupts and some other way of storing primask?

[MaJerle]

Very technically, primask shall always be stored as local variable. Can you point out how your call stack looks like when this happens?

[MaJerle]

Or maybe disable CO_CONFIG_FLAG_SDO_ALWAYS_LOCK_OD instead?

[somlioy]

Yeah primask stored in a local variable would be the optimal choice for such an event, altought I'm not sure how that would be done when using macros like that. A local variable inside the do-while would be limited to access within that scope.

See Call stack below (breakpoint at the point where I call OC_LOCK_OD (in storeEeprom).
So CO_LOCK_OD is first called by CO_SDOserver_process() and then by me in storeEeprom()

I've already updated CANopenNode to latest version, so CO_CONFIG_FLAG_SDO_ALWAYS_LOCK_OD is deprecated. See commit attached in first post.

Technically I guess it safe to assume storeEeprom() and restoreEeprom() never would be called from anything else than via SDO such that the LOCKs I have in store-/restoreEeprom safely can be removed.

However there's still the "issue" where systicks are disabled doing a LOCK causing HAL_Delay to be stuck in a while-loop. Encountered this during my first implementation of the eeprom-writing function where HAL_Delay was used to allow for eeprom-page writing to be finished, thought this was fixed by polling the eeprom wether it is ready or not (HAL_I2C_IsDeviceReady).

[skpang]

I'm seeing this problem is well.

CO_LOCK_OD has a __disable_irq() function inside:
#define CO_LOCK_OD(CAN_MODULE) \ do { \ (CAN_MODULE)->primask_od = __get_PRIMASK(); \ __disable_irq(); \ } while (0)

Where as CO_UNLOCK_OD only set the mask:
#define CO_UNLOCK_OD(CAN_MODULE) __set_PRIMASK((CAN_MODULE)->primask_od)

Should there be an enable irq inside CO_UNLOCK_OD ?

[MaJerle]

No. __set_PRIMASK will enable the interrupts, if these have been enabled before __disable_irq() has been called.

[skpang]

Ok, somehow the __set_PRIMASK is not re-enabling the interrupts for me.

[MaJerle]

This will happen if this happens:

CO_LOCK_OD();
CO_LOCK_OD(); <--- previous value is overwritten
CO_UNLOCK_OD(); <--- invalid previous value 
CO_UNLOCK_OD(); <--- invalid previous value

So is there a case when LOCK is called twice in a sequence