fix: API module; delay sending commands to EvseManager until it is ready #856
Conversation
james-ctc
requested review from
hikinggrass,
Pietfried and
corneliusclaussen
as code owners
james-ctc
force-pushed
the
fix/api-hold-startup-until-ready
branch
from
9f2b3db
to
0d5c97e
Compare
james-ctc
force-pushed
the
fix/api-hold-startup-until-ready
branch
2 times, most recently
from
1fc956c
to
ce2b495
Compare
modules/API/API.cpp
Outdated
| bool notify{false}; | ||
| { | ||
| std::lock_guard lock(evse_manager_mux); | ||
| evse_manager_ready--; |
There was a problem hiding this comment.
These changes rely on the fact that each EvseManager publishes ready only once (which is currently given). Instead of using a counter that counts down you could actually check for each EVSE individually (e.g. by using a map).
There was a problem hiding this comment.
@Pietfried updated the approach so that an EVSE manager can be ready multiple times and not cause an issue.
modules/API/API.cpp
Outdated
| @@ -269,12 +269,16 @@ void API::init() { | |||
| std::vector<std::string> connectors; | |||
| std::string var_connectors = this->api_base + "connectors"; | |||
|
|
|||
| evse_manager_ready = this->r_evse_manager.size(); | |||
There was a problem hiding this comment.
For consistency reasons I would explicitly use this here since it's been used everywhere else too
| evse_manager_ready = this->r_evse_manager.size(); | |
| this->evse_manager_ready = this->r_evse_manager.size(); |
There was a problem hiding this comment.
this-> is commonly needed for lambdas. Elsewhere this-> should not be added unless is is actually required.
There was a problem hiding this comment.
I agree, I just think it might be better to have a consistent style and this has been used throughout this file to refer to class members not just in lambdas
modules/API/API.hpp
Outdated
| @@ -192,6 +193,12 @@ class API : public Everest::ModuleBase { | |||
| std::vector<std::thread> api_threads; | |||
| bool running = true; | |||
|
|
|||
| std::condition_variable evse_manager_cv; | |||
| std::mutex evse_manager_mux; | |||
| std::int16_t evse_manager_ready{0}; | |||
There was a problem hiding this comment.
I think it would be cleaner to use a bool instead of an integer here since this would better reflect the intent. I also think I would make an explicit check in the cpp file like so:
evse_manager_ready = this->r_evse_manager.size() == 0;
| std::int16_t evse_manager_ready{0}; | |
| bool evse_manager_ready{0}; |
There was a problem hiding this comment.
r_evse_manager.size() will never equal 0.
evse_manager:
interface: evse_manager
min_connections: 1
max_connections: 128
If there are 10 EVSE managers then r_evse_manager.size() == 10
evse_manager_ready is set to 10 and decremented when a manager becomes ready.
evse_manager_ready being 0 or less means that at least 10 ready messages have been received.
There was a problem hiding this comment.
Alright :) then I must have misunderstood how this variable works
james-ctc
force-pushed
the
fix/api-hold-startup-until-ready
branch
2 times, most recently
from
8c4e6e3
to
1c00ddc
Compare
corneliusclaussen
force-pushed
the
fix/api-hold-startup-until-ready
branch
from
1c00ddc
to
3f29150
Compare
james-ctc
force-pushed
the
fix/api-hold-startup-until-ready
branch
4 times, most recently
from
875ade3
to
31e76aa
Compare
corneliusclaussen
added
the
include-in-release
Signed-off-by: James Chapman <[email protected]>
Signed-off-by: James Chapman <[email protected]>
same EVSE manager Signed-off-by: James Chapman <[email protected]>
Signed-off-by: James Chapman <[email protected]>
james-ctc
force-pushed
the
fix/api-hold-startup-until-ready
branch
from
31e76aa
to
8f4430c
Compare
…ady (#856) * fix: API module; delay sending commands to EvseManager until it is ready Signed-off-by: James Chapman <[email protected]> * fix: allow for 128 EvseManagers Signed-off-by: James Chapman <[email protected]> * fix: approach updated to allow multiple `ready` events from the same EVSE manager Signed-off-by: James Chapman <[email protected]> * fix: update to latest main and fix compile/merge issue Signed-off-by: James Chapman <[email protected]> --------- Signed-off-by: James Chapman <[email protected]>
Describe your changes
The API module can send commands to EvseManager before the manager has completed initialisation.
When this happens there is a potential for the manager to dereference a null pointer and terminate.
This change holds requests to EvseManagers until they have all responded with
ready.Issue ticket number and link
Checklist before requesting a review