speeduino icon indicating copy to clipboard operation
speeduino copied to clipboard

Make sure we use the same size data types on all platforms (Needs split/rework)

Open dantob opened this issue 4 years ago • 2 comments

For example on AVR int is only 16 bits, but on 32 bit boards it's 32 bits. lets always use int16_t/uint16_t to be sure. long should always be 32 bits but again use int32_t/uint32_t. same with byte int8_t/uint8_t.

This will probably break EEPROM compatibility on 32bit boards, but it's probably already broken in tunerstudio? considering speeduino.ini is based on the EEPROM layout of AVR?

dantob avatar Feb 02 '21 21:02 dantob

This is something that I've considered moving to for a while, but from what I've seen there's not a huge benefit (Currently) and a lot of risk. As you say, the byte -> uint8_t shouldn't be an issue at all as a byte should ALWAYS be 8 bits and this is about 90% of this PR (by line).

This will probably break EEPROM compatibility on 32bit boards, but it's probably already broken in tunerstudio? considering speeduino.ini is based on the EEPROM layout of AVR?

I doubt this is correct. The EEPROM code essentially breaks everything down to single bytes anyway, so if something is an uint, it will get split into 2 bytes. If it happened to be a 4 byte value, then it will still get split into 2 bytes with any value above 65535 being discarded (Which shouldn't be a problem as it's assumed to have been a 2 byte value in the first place).

If you can see anywhere this isn't the case, please do let me know.

For this type of change to get pulled in, it needs to be broken up to be much smaller PRs. Eg doing one PR per .h/.ino and having tested each PR. Anything that 'will probably break...' has very little chance of getting pulled in.

noisymime avatar Feb 02 '21 22:02 noisymime

If you'd like I can drop it down to just https://github.com/noisymime/speeduino/pull/517/commits/06f90400dde3356c2270c61bad146e59bc85a5aa for now, which is ints only, I can split that into a separate pull for signed and unsigned. Is that small enough? +-500 lines

I doubt this is correct. The EEPROM code essentially breaks everything down to single bytes anyway, so if something is an uint, > it will get split into 2 bytes. If it happened to be a 4 byte value, then it will still get split into 2 bytes with any value above 65535 > being discarded (Which shouldn't be a problem as it's assumed to have been a 2 byte value in the first place).

Ok

dantob avatar Feb 02 '21 22:02 dantob