Some small fixes and improvements extracted from PR1238#1565
Some small fixes and improvements extracted from PR1238#1565fschrempf wants to merge 5 commits intomeshcore-dev:devfrom
Conversation
| #define STATE_IDLE 0 | ||
| #define STATE_RX 1 | ||
| #define STATE_TX_WAIT 3 | ||
| #define STATE_TX_WAIT 2 |
There was a problem hiding this comment.
I don't follow what this is supposed to fix?
There was a problem hiding this comment.
Did you see the commit message?
STATE_TX_WAITis meant to be a single bit but is defined as two bits,
therefore overlapping withSTATE_RX. This causes incorrect results
when the code uses the bits to handle the state machine.
Unless I'm getting it all wrong (which totally could be the case) you meant to use the STATE_* defines as single-bit masks for the static volatile uint8_t state variable. But 3 is not a single bit but two bits. Therefore currently if state is set to STATE_TX_WAIT, then state & STATE_RX will return true, which seems unexpected and looks wrong to me.
I don't now if this has any negative impact with the current implementation, but it certainly caused problems with the code I was working at here: a6ce03d.
And anyway, it seems like a good idea to fix this no matter if it is currently causing any issues or not as it has the potential to cause problems in the future.
There was a problem hiding this comment.
It looks like there is no actual impact at the moment, as checks for STATE_RX are currently all done with == to check for STATE_RX exclusively.
There was a problem hiding this comment.
Only STATE_INT_READY is intended as a bit mask. The others are enums.
There was a problem hiding this comment.
Ok, got it. I still think it is very easy to get wrong the way it is currently written. Please see the updated PR for a suggestion to make it a bit easier to understand.
The STATE_* defines are meant to be used as enumerations, except for STATE_INT_READY which describes a single bit flag. Improve the name of the flag to make the difference clearer. While at it also make the enumeration use a continuous increment without gaps. Signed-off-by: Frieder Schrempf <frieder@fris.de>
There is no reason to not use the reset pin as the RAK4630/31 module has it connected internally. Signed-off-by: Frieder Schrempf <frieder@fris.de>
If the radio driver state machine is not in receive mode, this means that processing of a packet is still in progress and we are not in an idle state. Signed-off-by: Frieder Schrempf <frieder@fris.de>
This makes the code easier to read and allows for easier changing of the hardcoded values. Signed-off-by: Frieder Schrempf <frieder@fris.de>
When a CLI command is issued through the serial interface, extend the timeout for going to sleep to give the user more time for issuing more commands. Signed-off-by: Frieder Schrempf <frieder@fris.de>
fschrempf
force-pushed
the
pr1238-extracted-fixups-and-improvements
branch
from
4400d5b to
07a223e
Compare
2 participants
While working on #1238
I discovered a bugI had a hard time understanding the logic of the state handling inRadioLibWrappers.cpp. Therefore this PR improves the readability of that part and a few other things which can be reviewed and merged separately.Fix subtle bug inRadioLibWrappers.cppstate machineRadioLibWrappers.cpphasPendingWork()which is used to decide when the system is idle and can enter powersaving modeconstexprvalues for powersaving timings instead of inline literals