-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor/better consistency #544
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
base: development
Are you sure you want to change the base?
Conversation
…clares it's dependencies, everything else is automatic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the ST-LIB infrastructure to improve consistency and enable higher-order domain composition. The refactoring introduces a declarative dependency system where domains specify their dependencies via a dependencies tuple, and initialization is automated through template metaprogramming.
Key Changes:
- Modified
inscribemethods to return the index fromaddcalls, enabling higher-order domains to properly track dependencies - Changed Init structs to accept configuration arrays as template parameters instead of runtime parameters
- Introduced automatic dependency resolution where domains declare dependencies via
using dependencies = std::tuple<...>
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Inc/ST-LIB.hpp | Implements automatic domain initialization with dependency resolution using tuple-based domain registry and compile-time configuration |
| Inc/HALAL/Models/GPIO.hpp | Updates GPIODomain to return index from inscribe, changes Config from tuple to struct, and adapts Init template signature |
| Inc/ST-LIB_LOW/DigitalOutput2.hpp | Adds dependencies declaration, updates inscribe to call gpio.inscribe(), and changes Init to accept configs as template parameter |
| Inc/ST-LIB_LOW/DigitalInput2.hpp | Adds dependencies declaration, updates inscribe to call gpio.inscribe(), and changes Init to accept configs as template parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void init() { | ||
| Domains domains; | ||
| std::apply( | ||
| [](auto &...d) { | ||
| (init_domain<std::remove_cvref_t<decltype(d)>>(), ...); | ||
| }, | ||
| domains); | ||
| } |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init function does not guarantee that dependencies are initialized before the domains that depend on them. The domains are initialized in the order they appear in the tuple, but there's no mechanism to ensure that GPIODomain is initialized before DigitalOutputDomain and DigitalInputDomain. If the tuple order is changed, initialization could fail. Consider adding explicit topological sorting of domains based on their dependencies, or at minimum add static assertions to verify the initialization order is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defeloper should just put the domain in the correct order. If it is deemed necessary, a topological sort in compile-time will be implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best way to test this is using a real example, if the wrong order makes it crash at runtime then imo the correct order should be enforced at compile time. Not just relying on the developer, if we are doing the whole compile time safety thing this check should definitely be in there as well.
If it doesnt crash at runtime then it might just be a hallucination of copilot and no further work is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most times it shouldn't matter, since the init shouldn't make use of other things. That is, in most cases, there are exceptions and it would not fail in compile time (though adding this check is quite easy, not so much for the topological sort, but still worthwile exploring)
Refactor to the ST-LIB new infrastructure for better overall consistency.
Implementes the following:
Higher order domains now call inscribe of the lower order domains instead of calling add with their entries. This makes even higher order domains possible (since the entrie has to be changed in the inscribe method to account for the indexes and so on).
This needs for the inscribe to return the result of the
addcall (it's index) so that higher order domains can store it.Makes all Init structs take the configs as template parameter. This allows for more metaprogramming stuff.
You no longer have to put every domain in ST-LIB.hpp a hundred times, you just put the domain in the "domains" tuple once, and put the dependencies (domains it depends on) of your domain inside your domain (a
using dependecies = std::tuple<GPIODomain>for example).