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

BMA421 and BMA425 integration RFC #8

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

Conversation

thecodechemist99
Copy link
Contributor

This is a first draft of the BMA421 and BMA425 integration as discussed in this issue.

Scope

I have implemented and briefly tested the modifications within my pinetime-rust project. There’s definitely more thorough testing needed but I haven’t been able to spend a lot of time on that project lately.

For now, I’d like to get your feedback on my implementation. Please let me know if there’s anything you would like to change or you have suggestions for improvements.

Overview

  • Added the config files for the BMA421 and BMA425 models
  • Added type aliases for all three models, this allows to reuse the existing code for all of them while also providing a more intuitive interface to prevent issues in implementation
  • Put the aforementioned changes in separate modules so they are only included for the configured model via feature flags
  • Added feature flags for all three models

Additional Notes

I already changed the package name in the draft to bma42x to prevent confusion with the modules, which I have named after the models.

@pyaillet
Copy link
Owner

pyaillet commented Apr 6, 2024

Hi!

Awesome work, thanks a lot :)

I won't have time to test it in the next two weeks, but I hope to be able to do it then.

In the meantime, I have two comments:

  • I need to adapt the CI job which by default tries to activate all features at once, maybe one build per feature could secure the build.
  • Maybe an explicit compile error could be added to explain how to use the features.

@thecodechemist99
Copy link
Contributor Author

  • I need to adapt the CI job which by default tries to activate all features at once, maybe one build per feature could secure the build.

Does this have any implications for me or is it merely meant as an information?

  • Maybe an explicit compile error could be added to explain how to use the features.

Great idea! I will look into this, thanks for the suggestion.

@pyaillet
Copy link
Owner

pyaillet commented Apr 8, 2024

  • I need to adapt the CI job which by default tries to activate all features at once, maybe one build per feature could secure the build.

Does this have any implications for me or is it merely meant as an information?

It's only an information, I will do it on my side. You might have to rebase your branch once this is done though. I'll keep you informed

@thecodechemist99
Copy link
Contributor Author

thecodechemist99 commented Apr 9, 2024

Hi,

I added the compiler errors as requested.
Unfortunately there’s no clean way to check if multiple features are enabled. The best option I could find has been suggested here, but as this really is only a workaround, and to be more detailed about the features causing the error, I decided to add separate conditions for each device to the config files.
Let me know if you agree with this decision.

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