search

ramapcsx2 / gbs-control

GNU General Public License v3.0
788 stars 110 forks source link

Fix compile error with more recent ESP8266 toolchains #288

Closed M-Reimer closed 2 years ago

M-Reimer commented 2 years ago

With latest toolchains compiling fails in "framesync.h" at a position where the absolute value of the difference is calculated.

The problem with this is that both values, that are subtracted, are unsigned values. I don't know if older compilers had some workaround in place here, but this is clearly wrong. If you subtract a bigger unsigned integer from a smaller unsigned value, then this does not get negative but it will overflow and the result is clearly not what is expected here.

I've replaced this line with something that at least I think tells clearly what is meant here: "Subtract the smaller of the two variables from the bigger of the two variables".

ramapcsx2 commented 2 years ago

Hey, this is probably correct. The code I wrote looks like I wanted to make it fast, but I don't remember now. Do you think your version will be slower? It might matter a lot in this place.

M-Reimer commented 2 years ago

It may be a bit slower depending on how the compiler optimizes. If performance matters here, I'll update this PR later with a version that could be slightly faster.

ramapcsx2 commented 2 years ago

Okay, ultimately, it works at CPU counter speeds, so I realize that running comparisons with clamping and such is longer than the actual allowed "jitter" there. If you make it quite fast, it will be alright :)

M-Reimer commented 2 years ago

Can you please have a look at my latest commit? This should be about as fast as we can go.

ramapcsx2 commented 2 years ago

Hey, I just got back from a trip. I'll check it out soon!

LawnMo commented 2 years ago

Wouldn't it be easier and preserve what was initially meant by kanging ABS() from : https://github.com/MarlinFirmware/Marlin/commit/99ecdf59af907ebb8d2d847863614094bb576e3f#diff-a426db550c957a2391fe722b87be2b9d666b39aad58d382be93670b0a25167e8R197 ?
Haven't tried the actual change, but at least it compiles with latest toolchains.

M-Reimer commented 2 years ago

Wouldn't it be easier and preserve what was initially meant by kanging ABS() from [...]? Haven't tried the actual change, but at least it compiles with latest toolchains.

The only reason why this works is that the compiler does not know "ABS()" so the error check will not trigger.

The macro, used by the Marlin firmware, also does not "magically allow" to subtract a bigger from a smaller unsigned integer and get a negative value. C(++) just doesn't work this way. If you subtract unsigned integers, you get an unsigned integer. If the value, you subtract from, is smaller, then it overflows. The "ABS()" macro is just a faster (probably not even in all situations and heavily dependent on compiler version) variant of the original "abs()".

Of course it would be possible to make my current variant a bit "nicer" by wrapping it into a macro. For example called "ABS_DIFF" or similar. This macro would have to have two parameters as subtracting them is still wrong for unsigned values. But as it seems to be used only once, I did not do that.

ramapcsx2 commented 2 years ago

Sorry again for the delay. Your change is good, of course :)

I have to say that this code is ultimately relying on the speed of the ISR, how quickly the hardware reacts to a pulse and how quickly then the ISR code gets to execute. It is probably tuned for the ESP + software as it was back then, which may or may not have changed by now. All this is to say, it should be checked whether it still does the job :)