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

Improve LREAL support (WIP) #10

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lawrencegsullivan
Copy link
Contributor

@lawrencegsullivan lawrencegsullivan commented Feb 14, 2024

What

Don't demote doubles to floats in varGetValue

Why

Greater precision is better, maybe?
Sometimes we want to display double precision numbers on our HMIs

If y'all need double precision this might be the right starting place, however there may be additional changes required that I haven't considered yet.

I have not tested in the LoupeUX stack.

@lawrencegsullivan lawrencegsullivan added the enhancement New feature or request label Feb 14, 2024
@lawrencegsullivan lawrencegsullivan changed the title Feature/improve lreal support Improve LREAL support Feb 14, 2024
@lawrencegsullivan
Copy link
Contributor Author

Within the LoupeUX stack there are comms bandwidth considerations here as well.

@lawrencegsullivan lawrencegsullivan changed the title Improve LREAL support Improve LREAL support (WIP) Feb 14, 2024
@shanereetz
Copy link
Member

Conceptually this change seems like an improvement, in that the previous behavior seems to prevent users from getting LREALs into traces, or other parts of the HMI.

The bandwidth considerations are also important, and how this might impact existing applications if they were to update, especially if they are using many LREALs that are currently being demoted. Could we test performance with an existing application that uses many LREALs for comparison?

Lastly, I noticed that this PR has the TLSF updates as well, was that intentional or meant to be in a separate PR?

@shanereetz
Copy link
Member

Actual last thought: would it be worth offering some kind of flag to opt-in to LREAL support?

@lawrencegsullivan
Copy link
Contributor Author

Lastly, I noticed that this PR has the TLSF updates as well, was that intentional or meant to be in a separate PR?

StringExt v0.15.0 uses TLSF and builds with it as a static library. StringExt binaries were brought into the example project including a tlsf header file. This shouldn't impact VarTools.

@shanereetz
Copy link
Member

LGTM but given my minimal experience with this repo, I'll pass approval to someone with more internal knowledge. Thanks for doing this!

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

Successfully merging this pull request may close these issues.

2 participants