Use RadioLibTime_t (aka unsigned long) when dealing with millis() and micros() #1075
Conversation
Although sizeof(uint32_t) == sizeof(unsigned long) on Arduino, this is not the case on 64-bit Linux, where sizeof(unsigned long) == sizeof(uint64_t). Most timestamp arithmetic and comparisons have been left alone, to reduce code churn. This is fine, as uint32_t is perfectly wide to store most timestamp deltas this library will deal with, and C will promote the integer rather than do a narrowing conversion. The real problem arises with narrowing conversions being done by assuming timestamps are 32-bit. No functional changes intended for platforms where sizeof(uint32_t) == sizeof(unsigned long) (so most 8/16/32-bit platforms). Signed-off-by: Elizabeth Myers <[email protected]>
|
Thank you for the contribution! One question/suggestion, would it be possible to use a custom type for this, to make the intent clear? For example |
|
That's a great idea. I thought about doing it like this in fact. I will do it today after work. |
In that case it may also be beneficial to change more uses where timestamps are involved to use this type. I already had to update some of the API, it might be beneficial to just rip the bandaid off and just change it all for clarity. |
Definitely, that would be better to keep everything consistent. |
This makes it obvious what is and isn't a timestamp. Not everything has been converted; anything dealing with protocol and chip-level timestamps has been left alone on purpose, to make it clear that these functions do require 32-bit timestamps. No functional changes intended on platforms where sizeof(uint32_t) == sizeof(unsigned long). Signed-off-by: Elizabeth Myers <[email protected]>
I have mostly done so. I have kept some timestamps 32-bit, mostly stuff dealing with chips and protocol stuff. This is to make it clear what values are expected. |
Elizafox
changed the title
We need to not overflow the integers with the shifts and multiplications, so this is correct behaviour. Signed-off-by: Elizabeth Myers <[email protected]>
|
All looks good now - merged, thank you for the contribution! |
Although
sizeof(uint32_t) == sizeof(unsigned long)on Arduino, this is not the case on 64-bit Linux, wheresizeof(unsigned long) == sizeof(uint64_t).Most timestamp arithmetic and comparisons have been left alone, to reduce code churn. This is fine, as
uint32_tis perfectly wide to store most timestamp deltas this library will deal with, and C will promote the integer rather than do a narrowing conversion. The real problem arises with narrowing conversions being done by assuming timestamps are 32-bit.No functional changes intended for platforms where
sizeof(uint32_t) == sizeof(unsigned long)(so most 8/16/32-bit platforms).