Conversation
4e9afb1 to
2506b63
Compare
There was a problem hiding this comment.
One thing in advance concerning naming before I have a closer look: I'm not sure EVerest_API is the best name for this feature since it could quite easily be confused with the broader term we use for the "EVerest API" itself. Maybe entrypoint_API / introspection_API / metadata_API?
2506b63 to
72c0e9f
Compare
james-ctc
left a comment
There was a problem hiding this comment.
I'm not sure I follow how this is discovering the charger configuration and which API modules have been loaded by the manager.
How is the current charger configuration file detected?
My main suggestion is to try and make this as generic as possible so that new APIs can be easily added.
An alternative would be for every API instance to listen for a request and respond.
It would be nice if this solution didn't require another process to be started.
In particular this really needs to run regardless of charger configuration.
Another suggestion is to include the EVerest version in the response.
| ApiTypeEnum: | ||
| description: All known API types | ||
| type: string | ||
| enum: |
There was a problem hiding this comment.
I suggest using a string, otherwise changing this enum could result in breaking changes to the stable APIs. It should be safe for a new API type to be introduced without needing this file to be updated
There was a problem hiding this comment.
The question is how to communicate to the client which types actually exist (e.g. is it "powermeter" or "powermeter_API"). But I see your point and maybe a string suggests a more defensive programming style to the client developer who is required to expect and handle any possible string.
| session_cost_consumer, | ||
| slac, | ||
| system | ||
| }; |
There was a problem hiding this comment.
adding a new stable API would mean this needs updating. (a breaking change perhaps?)
| struct ApiParameter { | ||
| ApiTypeEnum type; | ||
| std::string module_id; | ||
| std::optional<int32_t> version; |
There was a problem hiding this comment.
can the version be anything other than 1?
everest_api/1/entrypoint_api implies that entrypoint_api is part of API version 1.
I'm not sure it should report on other versions.
Alternatively it could be version-less inn which case everest_api/entrypoint_api returning about all modules and versions would be useful
There was a problem hiding this comment.
The version-less api-discovery would be ideal (and changing the topic to your suggestion would be no problem), the problem today is, that you cannot know the other API-module's versions. (only the modules themselves know, but that piece of information isn't exposed via any interface nor via EVerest's configuration service).
|
Thanks for these helpful comments.
The manager reads the configuration file as usual (whichever it is) and provides module configuration for individual modules via the configuration service during runtime. This new module uses Everest::config::ConfigServiceClient to get the full configuration of the running EVerest instance and determines which of the loaded modules are EVerestAPI modules.
You suggest that each API instance listens to the same topic (say "everest_api/entrypoint_api") and replies to the topic given by the client via the replyTo field? So instead of a single reply with an array containing all the modules, the client would receive a burst of small individual replies each with a (name,type,version)-tuple. While my current approach also doesn't need to know anything about the configuration it does
The distributed approach solves all of this, as far as I can see. Also each modules listening to the outside world will know if it's part of this API or not and the version it implements.
Yes. That's on my list. |
|
Thank you for your comprehensive replies. One additional thought I had was whether the reporting could be integrated into the existing modules? i.e. It could work alongside this approach - because this could be extended to include getting information on how EVerest is configured and not just the stable APIs. That could help identify miss-configuration e.g. there are two EVerest EvseManagers but only one of them has a stable API connection |
…he everst_api_types lib Signed-off-by: Christoph Burandt <christoph.burandt@pionix.de>
- move EVerestAPI related modules to separate folder inside API/ - create a common base class for all EVerestAPI modules - change power_supply_DC_API and display_message_API modules to use common base + implement entrypoint_API for them Signed-off-by: Christoph Burandt <christoph.burandt@pionix.de>
72c0e9f to
aeaae5e
Compare
|
New approach:
Known issues:
Happy to get more input on this (@james-ctc, @mlitre) |
Signed-off-by: Christoph Burandt <christoph.burandt@pionix.de>
Signed-off-by: Christoph Burandt <christoph.burandt@pionix.de>
1130e16 to
68c55df
Compare
Describe your changes
Introduce an entrypoint_API definition which allows to query an EVerest instance for its EVerestAPI capabilities.
Also includes the EVerestAPI module providing this API and an example configuration.
Issue ticket number and link
Checklist before requesting a review