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

32bit regression #96

Open
zakk4223 opened this issue Jun 7, 2024 · 3 comments
Open

32bit regression #96

zakk4223 opened this issue Jun 7, 2024 · 3 comments

Comments

@zakk4223
Copy link

zakk4223 commented Jun 7, 2024

I'm using lg4ff on an armv7l system (32bit) and it looks like this commit 2b865af
regressed the previous 32bit fix.

I'm getting what I think is the same behavior that was originally reported in #47. FF_CONSTANT direction is mostly ignored, wheel tends to rotate to the left no matter what. Same with FF_PERIODIC.

I've confirmed that changing the line to
parameters[0].level = (long)parameters[0].level * (int)gain / 0xffff;
fixes it. Not sure if that re-introduces the clipping problem tho.

@berarma
Copy link
Owner

berarma commented Jun 7, 2024

Are you sure that the issue isn't that the forces are reversed?

Your change casts an unsigned 32bit value to a signed 32bit value, and that wouldn't have any effect in this case.

Unless armv7l is using 16bit values, then your change would produce an inversion of the forces.

@zakk4223
Copy link
Author

zakk4223 commented Jun 8, 2024

I think I sorted it out. the (int) cast was just masking the actual issue.

on arm arm7l int and long are 32 bits, long long is 64. The int cast was suppressing the implicit conversion of level to an unsigned value for arithmetic.

Using long long and doing the calculation like this fixes it:

parameters[0].level = div_s64((long long)parameters[0].level * gain, 0xffff);

I think the calculation of k1 and k2 in the loop below this also need this change.

I've verified this works on both 32 bit arm and 64bit x86

@berarma
Copy link
Owner

berarma commented Jun 8, 2024

These operations can be done with 32bit signed integers, the values they use are 16bit. Indeed, the result will be stored in a 32bit signed variable in 32bits systems.

I've created PR #97 to test whether there can be something else going on and avoid using 64bit types if possible.

berarma added a commit that referenced this issue Jun 18, 2024
Fixes #96.
32bits compatibility regression.
berarma added a commit that referenced this issue Jun 22, 2024
Fixes #96.
32bits compatibility regression.
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

2 participants