Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cases where int is assumed to be 32 bits and two's complement #17

Open
liampwll opened this issue Sep 22, 2021 · 2 comments
Open

Cases where int is assumed to be 32 bits and two's complement #17

liampwll opened this issue Sep 22, 2021 · 2 comments

Comments

@liampwll
Copy link

I've come across a few cases where you've written code that assumes int is 32 bits and/or two's complement, I think it would be reasonable to use int32_t in most of these cases as it's already used in other parts of the library. Here's a few examples I've come across but this obviously doesn't cover everything:

https://github.com/trinamic/TMC-API/blob/fd6a5378b2157bf115b8126c9183364b899fa27f/tmc/ic/TMC2160/TMC2160.c#L20

https://github.com/trinamic/TMC-API/blob/fd6a5378b2157bf115b8126c9183364b899fa27f/tmc/helpers/Macros.h#L33 (Here the shifts can overflow if the values are not cast to uint32_t first, additionally int32_t is passed to these macros where uint32_t should be used as >> and << are undefined behaviour on negative values and on some platforms perform sign extension of negative values which leads to the wrong result being returned)

https://github.com/trinamic/TMC-API/blob/fd6a5378b2157bf115b8126c9183364b899fa27f/tmc/ramp/LinearRamp.c (Here int is being used to store int32_t values, I haven't looked at this enough to know if it's an issue or not, additionally abs is being used on int32_t values)

@ClemensElflein
Copy link

I agree that this should be fixed. I had the same problem with the TMC6100. The code assumes that int is int32 but in my case it was int16.

@dhylands
Copy link

I'm trying to work an MSP430 and a TMC5160. The MSP430 has a 16-bit int, and I've found a couple other cases which I think need to be fixed:

https://github.com/trinamic/TMC-API/blob/4ced0291c5cdae49f54e7f082c3c651d03ca19e7/tmc/ic/TMC5160/TMC5160.h#L30
I replaced this int's with int32_t.

and the abs function takes an int as an argument and returns an int.
https://github.com/trinamic/TMC-API/blob/4ced0291c5cdae49f54e7f082c3c651d03ca19e7/tmc/ic/TMC5160/TMC5160.c#L235

I added a macro definition:

#define ABS(N) ((N<0)?(-N):(N))

and replaced abs with ABS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants