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

Fix float_to_fix64 return value & f32 trig function doc corrections #787

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

Text-Input
Copy link
Contributor

The current return argument for float_to_fix64() is f32, which doesn't make sense and is at odds with the definition in the pico SDK. This PR replaces it with the correct type.

The function as exposed by the SDK is here:
https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_float/include/pico/float.h#L44
It returns an int64, so I've updated rom_data.rs to match. I tested this on my pico by converting a small set of f32s to fix64 with the change in this PR, then back through fix64_to_float() and the values match up.

This is a breaking change, though given that the return argument was both the wrong type and the wrong size, I'm not sure if anyone could really have used this function as it currently stands.

Additionally, the function descriptions for fcos, fsin, and ftan state a valid range of -1024 to 1024, but the datasheet gives a range of -128 to 128 on page 140. Note that the valid range for doubles is specified as -1024 to 1024 on page 143.

The current return argument for float_to_fix64 doesn't
make sense and is at odds with the definition in the SDK.
Replace it with the correct type.

Similarly, the function descriptions for fcos, fsin, and ftan
give a valid range inconsistent with the values given
on page 140 of thedatasheet. This corrects them.
@jannic
Copy link
Member

jannic commented Apr 2, 2024

Hi David, thanks for this patch. I agree that this is technically a breaking change, but doesn't require a semver bump as it's strictly a bug fix, and I don't see how the buggy function could have been used in a sensible way. If this breaks some code, it's probably a good thing as the calling code was also buggy in the first place.

@jannic jannic merged commit 7f13938 into rp-rs:main Apr 2, 2024
6 checks passed
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

Successfully merging this pull request may close these issues.

2 participants