search

dgatf / msrc

Multi Sensor for RC with RP2040 - FrSky D, SmartPort, XBUS, SRXL, IBUS, SBUS, Multiplex Sensor Bus, Jeti Ex Bus, Hitec
GNU General Public License v3.0
168 stars 41 forks source link

Fix usage of millis() to avoid overflow #41

Closed MJ666 closed 2 years ago

MJ666 commented 3 years ago

This PR fixes issues with causes an overflow of the casted millis() usage every ~64 seconds. This overflow will potentially create problems in some situations.

MJ666 commented 3 years ago

Rebased to master.

dgatf commented 3 years ago

You're right. Thank you

The issue is that without the casting the flash size increases about 238 bytes and for the atmegas with 32k there are not many bytes left

To avoid this, could you please keep the casting but after the aritmetic operation:

(uint16_t)(millis() - ts)

In this way the flash size is not incremented and the results are correct

MJ666 commented 3 years ago

I have added back casts if possible and rebased the code to actual master again. Here are my results compiling for the Arduino Nano. The difference is now 152 bytes increase only. This is 86 bytes of saving from initial pull request.

Modified code with casts where possible: Der Sketch verwendet 29918 Bytes (97%) des Programmspeicherplatzes. Das Maximum sind 30720 Bytes. Globale Variablen verwenden 915 Bytes (44%) des dynamischen Speichers, 1133 Bytes für lokale Variablen verbleiben. Das Maximum sind 2048 Bytes.

Orginal code: Der Sketch verwendet 29766 Bytes (96%) des Programmspeicherplatzes. Das Maximum sind 30720 Bytes. Globale Variablen verwenden 913 Bytes (44%) des dynamischen Speichers, 1135 Bytes für lokale Variablen verbleiben. Das Maximum sind 2048 Bytes.

dgatf commented 3 years ago

Sorry, I didn't explain myself correctly. Also the timestamp variables can stay as uint16. The only change needed are the brackets, which are correct now

MJ666 commented 3 years ago

I guess this is what you had in mind? Found a nice article here wich also will work for the the casted millis() values in the same way.

https://www.norwegiancreations.com/2018/10/arduino-tutorial-avoiding-the-overflow-issue-when-using-millis-and-micros/

Thinking more about this likely your original code should work as intended. Now we calculate with the full resolution actual millis() value with the casted stored millis() value and cast the result. This likely also causes wrong calculation again?

dgatf commented 2 years ago

I guess this is what you had in mind? Found a nice article here wich also will work for the the casted millis() values in the same way.

https://www.norwegiancreations.com/2018/10/arduino-tutorial-avoiding-the-overflow-issue-when-using-millis-and-micros/

Thinking more about this likely your original code should work as intended. Now we calculate with the full resolution actual millis() value with the casted stored millis() value and cast the result. This likely also causes wrong calculation again?

Finally found that (uint16_t)millis() - ts was failing only for ARM Cortex and not ATmega. This was confusing.

Changed to (uint16_t)(millis() - ts) as proposed in this PR

Fixed with ffb0ed3b82eb31d2e03882600988e590c5ea60fb to avoid rebasing again

Thanks!