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

Ensure float operations in SoundSynthesizerEffects & CompassCalibrator. #454

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

microbit-carlos
Copy link
Collaborator

@microbit-carlos microbit-carlos commented Nov 25, 2024

Related PR, which includes files in codal-core like LevelDetectorSPL, and together with this PR it saves >400 bytes of flash:

Edit: Also ensured float literals in a couple of other files to avoid implicit double promotion.

@microbit-carlos microbit-carlos force-pushed the float-math-funcs branch 2 times, most recently from f0ea2e4 to a917616 Compare November 26, 2024 17:44
@microbit-carlos microbit-carlos changed the title Ensure float operations in SoundSynthesizerEffects. Ensure float operations in SoundSynthesizerEffects & CompassCalibrator. Nov 26, 2024
@lancaster-university lancaster-university deleted a comment from github-actions bot Nov 26, 2024
@lancaster-university lancaster-university deleted a comment from github-actions bot Dec 18, 2024
@lancaster-university lancaster-university deleted a comment from github-actions bot Dec 18, 2024
@lancaster-university lancaster-university deleted a comment from github-actions bot Dec 18, 2024
@microbit-carlos
Copy link
Collaborator Author

microbit-carlos commented Dec 18, 2024

As the PR in codal-core has been merged, I've udpated target-locked.json to point to that commit and that shows the half KB of flash saving from having fewer/smaller math functions included in the build.

Avoid implicit int promotion to doubles when the final results are
stored in floats, and use float versions of math function when
the results are stored in floats as well.
Copy link

Build diff

Base commit: 73329cea9f963fe3129f7f51c2a668dc50c952e5
Action run: https://github.com/lancaster-university/codal-microbit-v2/actions/runs/12400645876

     VM SIZE    
 -------------- 
  +1.9%     +24    /home/runner/work/codal-microbit-v2/codal-microbit-v2/libraries/codal-core/source/streams/LevelDetectorSPL.cpp
  -2.1%     -16    /home/runner/work/codal-microbit-v2/codal-microbit-v2/libraries/codal-core/source/streams/StreamNormalizer.cpp
  -5.7%     -56    /home/runner/work/codal-microbit-v2/codal-microbit-v2/libraries/codal-core/source/driver-models/Compass.cpp
  -2.1%    -480    [section .text]
  -0.1%    -528    TOTAL

@finneyj
Copy link
Contributor

finneyj commented Feb 18, 2025

Looks good - I'm not sure we want to include the target-locked.json though do we @microbit-carlos?

@microbit-carlos
Copy link
Collaborator Author

While it is not really necessary for this PR, I actually do prefer when commits going to codal-microbit-v2 update the target-locked.json file IF they need a commit from any of its dependencies.

Normally the target-locked.json is only updated when a new tag has been created, and that has in some occasions caused microbit-v2-samples to have broken builds.
This happens when the main branch is in that in-between state after merging everything into all the codal repositories, but before creating a codal-microbit-v2 tag with the updated target-locked.json. This has also been a bit annoying when trying to go back to old commits for testing or doing a git bisect, and having to guess what the right codal-core/codal-nrf52 commits were that worked with that specific commit from codal-microbit-v2.
On the other hand, doing this can create merge conflicts if multiple PRs do that, but I'd probably argue that is easy enough to resolve, and having the main branch always in working state does feel worthwhile.

That doesn't strictly applies to this PR though, as it doesn't really need lancaster-university/codal-core@b395ae3 to compile, so happy to remove it in this case.

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