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

optimisation: calculate steps per unit reciprocal at compile time #336

Closed
wants to merge 1 commit into from

Conversation

gudnimg
Copy link
Collaborator

@gudnimg gudnimg commented Dec 22, 2024

Using the reciprocal allows us to use multiplication instead of division at runtime.

This commit removes 7 divison operations at run time.

Change in memory:
Flash: -210 bytes
SRAM: 0 bytes

Copy link

github-actions bot commented Dec 22, 2024

All values in bytes. Δ Delta to base

ΔFlash ΔSRAM Used Flash Used SRAM Free Flash Free SRAM
-210 0 27922 1667 750 893

@DRracer
Copy link
Collaborator

DRracer commented Dec 22, 2024

Interesting, even though you are introducing new variables, the resulting code is significantly shorter than before, good job. Please have a look at the unit tests failing .... oh, it's just the comment 🤦

Run mshick/[email protected]
Error: Resource not accessible by integration

@gudnimg
Copy link
Collaborator Author

gudnimg commented Dec 22, 2024

Please have a look at the unit tests failing .... oh, it's just the comment 🤦

Yup it is causing all PRs to fail. Only someone with admin access to the repo can fix it. We had the same problem on Prusa-Firmware repo before.

@DRracer
Copy link
Collaborator

DRracer commented Dec 23, 2024

@3d-gussner has admin rights 😉

@gudnimg
Copy link
Collaborator Author

gudnimg commented Dec 23, 2024

@3d-gussner it looks like some of the unit tests are timing out on this PR. Not sure if its random, but I don't see it in the other PRs. I haven't looked into if the unit tests need updating.

@gudnimg gudnimg marked this pull request as draft December 24, 2024 00:13
@3d-gussner 3d-gussner requested a review from DRracer December 24, 2024 06:14
Using the reciprocal allows us to use multiplication instead of division at runtime.

This commit removes 7 divison operations at run time.

Change in memory:
Flash: -210 bytes
SRAM: 0 bytes
@gudnimg gudnimg force-pushed the opt-gudni-divisions branch from efc2ce1 to 87a1862 Compare December 24, 2024 13:47
@3d-gussner
Copy link
Collaborator

3d-gussner commented Dec 24, 2024

All motion tests fail. Also testing with M404 results in IDLER CANNOT HOME
Before PR 107 of 107 tests passed.
After PR 63 of 107 tests passed.

@gudnimg
Copy link
Collaborator Author

gudnimg commented Dec 24, 2024

I think I see what the problem is... its with (T)axisScale[AU::axis].stepsPerUnitReciprocal. Where T is int32_t. And since axisScale[AU::axis].stepsPerUnitReciprocal is less than 1 casting this into integer gives just 0.

@gudnimg gudnimg closed this Dec 25, 2024
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.

3 participants