-
Notifications
You must be signed in to change notification settings - Fork 52
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
V1.6 in Pending state: Handling of the TriggerMessage.req to set connection state to accepted #421
Conversation
Signed-off-by: Matthias Suess <[email protected]>
Signed-off-by: Matthias Suess <[email protected]>
Signed-off-by: Matthias Suess <[email protected]>
Signed-off-by: Matthias Suess <[email protected]>
In order to accept TriggerMessage.req to send out the relevant requested *.req e.g. Heartbeat requires a bit of refactor as there can happen a interference with calls comming from outside. Stacked variable would be preferred to manage acceptance to allow sending of the package, although, it is in pending state. But this requires to change the calls eg. heartbeat(bool xAccept) and send(ocpp::Call call, bool xAccept), the same with status_notification and send_meter_value. |
if (enhanced_message.messageType == MessageType::GetConfiguration || | ||
enhanced_message.messageType == MessageType::ChangeConfiguration || | ||
enhanced_message.messageType == MessageType::TriggerMessage) { |
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 this is too restrictive, the 1.6 spec in section 4.2 states
While in pending state, the following Central System initiated messages are not allowed:
RemoteStartTransaction.req and RemoteStopTransaction.req
There are no other restriction (other than not issuing chargepoint initiated requests)
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.
@bWF0dGhpYXMK what's your opinion on this?
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.
Hi, a review of the latest code, there is a PR #520 managing that case with an answer on Rejected. From the ocpp1.6, for me it is not clear what means not allowed, initially I thought on "no" answer, which is two edged. Because an on the RPC framework point of view you require at least an answer such as Call Error.
So on that level, i think this PR can be closed. Either reject or CALL Error.
It remains the clarification on ReserverNow.req, Reset.req, ... what behavior here is the most likely in the sense of the describtion.
From this " The Central System MAY send request messages to retrieve information from the
Charge Point or change its configuration. The Charge Point SHOULD respond to these messages.". It sound like behave as in Remote(Start/Stop).req...
lib/ocpp/v16/charge_point_impl.cpp
Outdated
if (this->connection_state = ChargePointConnectionState::Pending) { | ||
EVLOG_debug << "Set connection_state from pending to accepted"; | ||
this->connection_state = ChargePointConnectionState::Booted; | ||
} |
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.
How do you come to this conclusion? From my understanding only the BootNotification.Conf should change this.
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.
Thank you for review. Initially, I interpretated that sentence: "The Charge
Point SHALL NOT send request messages to the Central System unless it has been instructed by the Central
System to do so with a TriggerMessage.req request." To use any TriggerMessage.req as trigger to set the connected state. However, after discussion, the sentence more reflects, the *.req initiated by a TriggerMessage.req. In that sense, the code change is not needed anymore. As proposed by you.
Signed-off-by: Matthias Suess <[email protected]>
Following discussions on Issue 478 related to the topic, the PR is set as done/closed. |
If the answer of backend is bootnotificaiton.conf pending, then a TriggerMessage.req shall set the charge point to booted.