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

Docs: Prefer module-specific settings over board configs #161

Open
ghost opened this issue Aug 11, 2022 · 7 comments
Open

Docs: Prefer module-specific settings over board configs #161

ghost opened this issue Aug 11, 2022 · 7 comments
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@ghost
Copy link

ghost commented Aug 11, 2022

EDIT:
This pattern would be a good thing to add to the Development docs so others understand how and why things are done this way.

Building on top of #159 and #160, it would be better to remove the board.config["krux"] pattern and instead have the modules themselves define settings that they reference to make decisions, where the default value of a setting could be based on the board.config["type"] or similar non-Krux-specific properties.

This would:

  1. Let us clean up board configs in MaixPy so we can remove the krux key and once again match upstream MaixPy's versions
  2. Keep board-specific logic very close to where it matters, in the module that uses it
  3. Allow users to override particular settings as they see fit and have them persist on microSD

This would also resolve #147 nicely.

Using that as an example, if display.py had a DisplaySettings class with an inverted_coordinates property, the default value could be initially set based on if board.config["type"] == 'amigo_tft'. The calling code could then say if settings.display.inverted_coordinates: <do stuff>, and users could go into their Settings to override this if they wanted.

@ghost ghost added enhancement New feature or request good first issue Good for newcomers labels Aug 11, 2022
@ghost ghost added this to the Next Major Release milestone Aug 11, 2022
@ghost
Copy link
Author

ghost commented Aug 13, 2022

@odudex See my work in #164 for examples of the new pattern, particularly thermal.py

@odudex
Copy link
Member

odudex commented Aug 14, 2022

@jreesun , looks good!
Do you think I should create a setting covering cameras? I think no setting is necessary for them. The way I did cameras are "plug and play". There's nothing to set, neither by user, neither by any hard code. Maixpy already does the job of finding out what camera is being used and the few necessary adjustments are constrained in camera.py file

@ghost
Copy link
Author

ghost commented Aug 15, 2022

What you did with cameras makes sense to me! When there's no possible variation, a setting wouldn't apply. I just wanted to make sure you saw this if you decided to start refactoring display or other classes where user preference may apply.

@odudex
Copy link
Member

odudex commented Aug 15, 2022

Got it! Yes, I'm watching the changes and will follow the new pattern in case I change something.

@odudex
Copy link
Member

odudex commented Aug 15, 2022

Tested your latest code. It seems Maixpy can't handle __dict__

@ghost
Copy link
Author

ghost commented Aug 15, 2022

Oi! That's what I get for only testing in the simulator... I'll see what I can do, thanks

@ghost
Copy link
Author

ghost commented Aug 15, 2022

@odudex Fixed here: #167 Going to merge once tests pass

@ghost ghost added documentation Improvements or additions to documentation and removed enhancement New feature or request good first issue Good for newcomers labels Oct 5, 2023
@ghost ghost changed the title Move away from board configs in favor of module-specific settings Docs: Prefer module-specific settings over board configs Oct 5, 2023
@ghost ghost modified the milestones: Version 23.07.0, Next Release Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

1 participant