eclipse-threadx / threadx

Eclipse ThreadX is an advanced real-time operating system (RTOS) designed specifically for deeply embedded applications.
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/threadx/index.md
MIT License
2.87k stars 782 forks source link

tx_event_flags_get doesn't always return the actual flags #210

Closed nekomata-san closed 1 year ago

nekomata-san commented 1 year ago

When calling the tx_event_flags_get API, the actual flags are only set upon return if one of the requested event flags is set. So, if I want to keep a persistent flag Y and clear a different flag X, there is no way for me to do this in one API call if flag_X isn't currently set in the event "current flags." For example:

UINT result = tx_event_flags_get( &event_flags, myflag_X, TX_OR_CLEAR, &actual_flags, TX_NO_WAIT ); if ( result == TX_SUCCESS ) { / Do something when myflag_X is set. It is also now cleared. / } if ( actual_flags & myflag_Y ) { / Do something when myflag_Y is set but it persists when set. / }

As far as I can tell, I would need to do this by calling the API twice with different options and incurring twice the overhead when it would work fine if "actual flags" were always what it claims to be.

goldscott commented 1 year ago

Hi @nekomata-san, I am sorry that the documentation is unclear. We will improve it. The actual_flags parameter may not show set flags that aren't requested. You could use tx_event_flags_info_get(&event_flags, TX_NULL, &actual_flags, TX_NULL, TX_NULL, TX_NULL); to get the actual flags that are set.

nekomata-san commented 1 year ago

Hi Scott! Thanks for your reply.

This is a simple one liner fix within ThreadX to do the right thing by just setting the flags in either case. Then we don't need to incur unnecessary interrupt disabled overhead and it will match the documentation. Wouldn't it be better to just fix the bug rather than change the documentation to match buggy behavior?

Making an entirely separate API call just to get a 32 bit value you already have in the first API call seems like the wrong approach. Especially since you store the flags if one of them is set.

goldscott commented 1 year ago

Hi @nekomata-san - I just reviewed the history of ThreadX and it has always behaved this way. I agree with you that we should set the actual_flags in all cases, so I will modify the code and it should be available in our next release, scheduled for the end of October.

nekomata-san commented 1 year ago

Thanks Scott! I appreciate your help.

goldscott commented 1 year ago

Hi @nekomata-san - as of this 6.2.0 release, this issue has been fixed. See updated files here: https://github.com/azure-rtos/threadx/blob/master/common/src/tx_event_flags_get.c https://github.com/azure-rtos/threadx/blob/master/common_smp/src/tx_event_flags_get.c