-
Notifications
You must be signed in to change notification settings - Fork 48
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
B04 offline idle behaviour #168
Conversation
lib/ocpp/v201/charge_point.cpp
Outdated
if (!(conn_state_per_evse[{evse_id,connector_id}].connector_status == | ||
evse->get_state(connector_id))) { | ||
// if the state varied when offline | ||
conn_state_per_evse[{evse_id,connector_id}].is_status_changed_when_offline = true; |
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.
Why do we need to store this value for use later instead of using it directly?
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 choose this approach as the status notification cannot be sent until the registration status is accepted (see message_callback
). So I first check for the change in status in the connection callback and make note of the change in status if it had changed when offline. The registration can take a while and it's possible that the connector state might have changed again.
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.
What about just checking the registration status in the connection callback? When we are accepted we stay accepted even after a lost connection. If we were not accepted yet we can just drop the changes since we are required to send the status notifications of all connectors anyway after a BootNotificationResponse with accepted (B01.FR.05).
conn_states_per_evse_id_key conn_states_struct_key; | ||
conn_states_per_evse_id_value conn_states_struct_value; | ||
EVLOG_debug<< "LIBOCPP: evseId: " << evse_id << " numConn: " << number_of_connectors; | ||
for (int connector_id = 1; connector_id <= number_of_connectors; connector_id++) { |
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 don't like the fact that you need to iterate the connectors here manually. I know this is currently the only option, but maybe we can improve this by adding a getter function for the entire connector map of an Evse?
lib/ocpp/v201/charge_point.cpp
Outdated
|
||
this->offline_threshold_timer.interval( |
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 still think it is a cleaner solution to just store the timestamp here and compare it when going back online. Now we have a bool variable and an entire timer object stored and the functionality is just the same.
std::chrono::time_point<std::chrono::steady_clock>
should do the trick. It is a monotonic timer so no worries about it moving backwards or whatever.
lib/ocpp/v201/charge_point.cpp
Outdated
if (!(conn_state_per_evse[{evse_id,connector_id}].connector_status == | ||
evse->get_state(connector_id))) { |
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.
if (!(conn_state_per_evse[{evse_id,connector_id}].connector_status == | |
evse->get_state(connector_id))) { | |
if (conn_state_per_evse[{evse_id,connector_id}].connector_status != | |
evse->get_state(connector_id)) { |
fdb9dac
to
a91cac2
Compare
lib/ocpp/v201/charge_point.cpp
Outdated
|
||
// B04.FR.01 | ||
// If offline period exceeds offline threshold then send the status notification for all connectors | ||
if (is_offline_threshold_expired) { |
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.
What about?
if (is_offline_threshold_expired) { | |
auto offline_duration = std::chrono::steady_clock::now() - time_disconnected; | |
if (offline_duration >= offline_threshold_value) { |
Then you don't need is_offline_threshold_expired
anymore and you could also use the threshold value directly without storing anything?
Edit: You don't need the abs
since it is a monotonic timer. It will always only increase so it can never be the other way around
lib/ocpp/v201/charge_point.cpp
Outdated
@@ -450,7 +505,7 @@ void ChargePoint::init_websocket() { | |||
}); | |||
|
|||
this->websocket->register_message_callback([this](const std::string& message) { this->message_callback(message); }); | |||
} | |||
} |
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.
Please revert this ;)
236bd07
to
0aa1f65
Compare
include/ocpp/v201/charge_point.hpp
Outdated
@@ -126,6 +126,26 @@ class ChargePoint : ocpp::ChargingStationBase { | |||
int network_configuration_priority; | |||
bool disable_automatic_websocket_reconnects; | |||
|
|||
// store the connector status | |||
struct conn_states_per_evse_id_key { |
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.
This name seems a bit specific to what it is. Maybe evse_connector_pair
or unique_connector
or something? That way it can be used for other purposes too.
include/ocpp/v201/charge_point.hpp
Outdated
|
||
std::map<conn_states_per_evse_id_key, ConnectorStatusEnum> conn_state_per_evse; | ||
|
||
std::chrono::seconds offline_threshold_value; |
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.
This is no longer user right?
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.
which one? they're both used
lib/ocpp/v201/charge_point.cpp
Outdated
@@ -425,29 +425,78 @@ void ChargePoint::init_websocket() { | |||
|
|||
this->websocket = std::make_unique<Websocket>(connection_options, this->pki_handler, this->logging); | |||
this->websocket->register_connected_callback([this](const int security_profile) { | |||
// handle offline threshold | |||
// Get the current time point using steady_clock | |||
std::chrono::steady_clock::duration offline_duration = std::chrono::steady_clock::now() - time_disconnected; |
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.
Last thing for this, could you move it inside the if statement? That cleans this function up a bit more.
lib/ocpp/v201/charge_point.cpp
Outdated
for (auto const& [evse_id, evse] : this->evses) { | ||
evse->trigger_status_notification_callbacks(); | ||
} | ||
} else if (offline_duration <= offline_threshold_value) { |
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.
} else if (offline_duration <= offline_threshold_value) { | |
} else { |
lib/ocpp/v201/charge_point.cpp
Outdated
// B04.FR.01 | ||
// If offline period exceeds offline threshold then send the status notification for all connectors | ||
if (offline_duration > offline_threshold_value) { | ||
EVLOG_debug << "LIBOCPP: offline for more than offline threshold "; |
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.
Maybe remove the LIBOCPP:
part of your debug prints? I think the logger already prints which module it comes from
e84303c
to
33cb132
Compare
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.
Looks good apart from some minor styling remarks and a typo 👍
include/ocpp/v201/charge_point.hpp
Outdated
@@ -133,6 +133,24 @@ class ChargePoint : ocpp::ChargingStationBase { | |||
int network_configuration_priority; | |||
bool disable_automatic_websocket_reconnects; | |||
|
|||
// store the connector status | |||
struct evse_connector_pair { |
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.
we prefer CamelCase here
include/ocpp/v201/charge_point.hpp
Outdated
// store the connector status | ||
struct evse_connector_pair { | ||
int32_t evse_id; | ||
int32_t connectior_id; |
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.
typo
lib/ocpp/v201/charge_point.cpp
Outdated
|
||
// B04.FR.01 | ||
// If offline period exceeds offline threshold then send the status notification for all connectors | ||
if (offline_duration > (std::chrono::seconds)this->device_model->get_value<int>( |
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.
nit: Prefer not to cast but use
std::chrono::seconds(this->device_model->get_value<int>( ControllerComponentVariables::OfflineThreshold))
instead
lib/ocpp/v201/charge_point.cpp
Outdated
this->message_queue->pause(); | ||
|
||
// check if offline threshold has been defined | ||
if ((std::chrono::seconds)this->device_model->get_value<int>(ControllerComponentVariables::OfflineThreshold) != |
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.
Casting both to std::chrono::seconds not necessary here; Could just be
this->device_model->get_value<int>(ControllerComponentVariables::OfflineThreshold) != 0
include/ocpp/v201/charge_point.hpp
Outdated
@@ -133,6 +133,24 @@ class ChargePoint : ocpp::ChargingStationBase { | |||
int network_configuration_priority; | |||
bool disable_automatic_websocket_reconnects; | |||
|
|||
// store the connector status | |||
struct evse_connector_pair { |
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.
could also be defined publicly, I could imagine we could reuse this for other purposes. We could also move this later though
82ca988
to
1c9e32a
Compare
* Store the connector status when offline into a map * send status notification for changed connectors when back online Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> Signed-off-by: pietfried <pietgoempel@gmail.com>
1c9e32a
to
5b559d9
Compare
Offline Behavior Idle Charging Station