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

add compile-time options for all preferences to movement_config #295

Merged
merged 2 commits into from
Mar 17, 2024

Conversation

madhogs
Copy link
Contributor

@madhogs madhogs commented Oct 2, 2023

PR to add compile-time options for all preferences to movement_config.h.
I have set the values to match all currently existing defaults.

This will allow the user to omit the preferences face if they wish and define all options at build time.
Addresses issue - #291 (some of it at least)

@madhogs madhogs force-pushed the preferences_in_config branch from 9fd729d to 868fecd Compare October 2, 2023 16:23
Copy link
Collaborator

@matheusmoreira matheusmoreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! More compile time customization features is always good.

Further improvements can be built on top of this in the future: setting flags like these could be made to cause the settings variables to be deleted and their references turned into compile time constants. This will allow compilers to partially evaluate functions and to eliminate dead code automatically, making for a smaller and more efficient program.

matheusmoreira added a commit to matheusmoreira/sensor-watch that referenced this pull request Mar 5, 2024
Adds overridable C preprocessor definitions for every user preference.
Enables the user to set defaults and omit the preferences face.

The default behavior of the watch is preserved.

Suggested-by: Wesley Aptekar-Cassels <[email protected]>
Implemented-by: madhogs <[email protected]>
Reviewed-by: Matheus Afonso Martins Moreira <[email protected]>
Signed-off-by: Matheus Afonso Martins Moreira <[email protected]>
GitHub-Pull-Request: joeycastillo#295
GitHub-Related-Issue: joeycastillo#291
matheusmoreira added a commit to matheusmoreira/sensor-watch that referenced this pull request Mar 5, 2024
Adds overridable C preprocessor definitions for every user preference.
Enables the user to set defaults and omit the preferences face.

The default behavior of the watch is preserved.

Suggested-by: Wesley Aptekar-Cassels <[email protected]>
Implemented-by: madhogs <[email protected]>
Reviewed-by: Matheus Afonso Martins Moreira <[email protected]>
Signed-off-by: Matheus Afonso Martins Moreira <[email protected]>
GitHub-Pull-Request: joeycastillo#295
GitHub-Related-Issue: joeycastillo#291
matheusmoreira added a commit to matheusmoreira/sensor-watch that referenced this pull request Mar 5, 2024
Adds overridable C preprocessor definitions for every user preference.
Enables the user to set defaults and omit the preferences face.

The default behavior of the watch is preserved.

Suggested-by: Wesley Aptekar-Cassels <[email protected]>
Implemented-by: madhogs <[email protected]>
Reviewed-by: Matheus Afonso Martins Moreira <[email protected]>
Signed-off-by: Matheus Afonso Martins Moreira <[email protected]>
GitHub-Pull-Request: joeycastillo#295
GitHub-Related-Issue: joeycastillo#291
matheusmoreira added a commit to matheusmoreira/sensor-watch that referenced this pull request Mar 5, 2024
Adds overridable C preprocessor definitions for every user preference.
Enables the user to set defaults and omit the preferences face.

The default behavior of the watch is preserved.

Suggested-by: Wesley Aptekar-Cassels <[email protected]>
Implemented-by: madhogs <[email protected]>
Reviewed-by: Matheus Afonso Martins Moreira <[email protected]>
Signed-off-by: Matheus Afonso Martins Moreira <[email protected]>
GitHub-Pull-Request: joeycastillo#295
GitHub-Related-Issue: joeycastillo#291
@matheusmoreira
Copy link
Collaborator

Am now running this code on real hardware. Used it to set 24h as default. Worked. No issues.

@madhogs
Copy link
Contributor Author

madhogs commented Mar 10, 2024

@matheusmoreira That's great to hear, thanks for looking at this. Let me know if there is anything more needed to merge this in.

@matheusmoreira
Copy link
Collaborator

Let me know if there is anything more needed to merge this in.

I think the upstream maintainers are reviewing it right now. I'm looking forward to it.

In any case, it's already been merged into my branch¹ along with several other pull requests. I'm running that branch on my watch with no issues so far. You're welcome to use it if you'd like!

@theAlexes theAlexes merged commit f35cb84 into joeycastillo:main Mar 17, 2024
@madhogs madhogs deleted the preferences_in_config branch March 17, 2024 09:27
@814d3
Copy link

814d3 commented Mar 30, 2024

@madhogs, since there are "instant" LED activation and 10 min. LE timeout, there need to be some additions in your code, right? I'm not so familiar with this, so just a note.

@madhogs
Copy link
Contributor Author

madhogs commented Mar 30, 2024

@814d3 yes, this was merged after the 10min timeout option was merged and unfortunately needs changing. I have adjusted the options in my next pr here #387 . This fixes the comment and puts the default back to 1 hour.
The instant option hasn't been merged yet (i'm pretty sure) so it should be correct in my latest PR. If that does get merged first i will update my pr.

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.

4 participants