Feature/data transfer functional block #871
Conversation
…ypes and made it a free function. Removed unused imports and usage of boost/uuid headers Signed-off-by: Piet Gömpel <[email protected]>
…ng the targeted design. Added test cases for the new functional block Signed-off-by: Piet Gömpel <[email protected]>
Pietfried
force-pushed
the
feature/data-transfer-functional-block
branch
from
7d62682 to
9578416
Compare
Pietfried
requested review from
hikinggrass,
marcemmers and
maaikez
as code owners
| virtual std::optional<DataTransferResponse> data_transfer_req(const DataTransferRequest& request) = 0; | ||
|
|
||
| /// \brief Handles the given DataTransfer.req \p call by the CSMS by responding with a CallResult | ||
| virtual void handle_data_transfer_req(Call<DataTransferRequest> call) = 0; |
There was a problem hiding this comment.
For this functional block not really necessary but for the others it would be nicer if this would just be one call I think. Something like handle_message() that internally splits again based on the message type just like the charge_point handle_message.
That enables us to decouple a bit more in not having to change the charge_point as much for new messages or changing message handlers.
There was a problem hiding this comment.
Should probably then pass along the const EnhancedMessage<v201::MessageType>& message
There was a problem hiding this comment.
Ah right, I should have added a comment for this since you marked that already in the other PR. I will add it here and definitely agree for other functional blocks, but in the end that is handled/defined by every functional block individually, since the handle_message function is not defined in any general interface but in the interface of every functional block.
There was a problem hiding this comment.
Ah ok no worries. Yes it does not matter that much for this one but thought it would be nice for uniformity ;)
There was a problem hiding this comment.
I wanted to say the same, it would be nice to have a handle_message in every functional block.
There was a problem hiding this comment.
I changed this to a handle_message function and found it reasonable to add an interface for it. Using that interface would be more of a convention than something that we can enforce when implementing other functional blocks but it provides at least some soft guidance.
This change goes beyond the scope of the PR, but since we are in the the message dispatching / handling is tightly related to the first functional block implementing it, i would be in favor of merging this as part of this PR.
| ocpp::Call<DataTransferRequest> call(request); | ||
| auto data_transfer_future = this->message_dispatcher.dispatch_call_async(call); | ||
|
|
||
| if (not this->is_websocket_connected()) { |
There was a problem hiding this comment.
I don't really like that we have to pass this check along here. Especially since we also have enhanced_message.offline as a value that we return. Would be nice if we can remove the is connected callback and just use the other behavior instead.
If we really do need to have information if we are connected, maybe its better to abstract it into the message dispatcher instead since all blocks will have that anyway.
There was a problem hiding this comment.
I agree, I was just trying to move the functionality to the functional block and provide everything that is required. I think we can indeed just make use of enhanced_message.offline here.
|
Overall looks really good, I think we are heading in the right direction with this! Also good to see at least 100% line coverage on the new functional block files 👍 |
* DataTransferInterface inherits from this MessageHandlerInterface Signed-off-by: Piet Gömpel <[email protected]>
Signed-off-by: Piet Gömpel <[email protected]>
|
I added the changes discussed above, so it is again ready for review |
| req.data = data; | ||
|
|
||
| return this->data_transfer_req(req); | ||
| return this->data_transfer->data_transfer_req(vendorId, messageId, data); |
There was a problem hiding this comment.
Is this function still necessary then?
There was a problem hiding this comment.
Yes since it doesnt handle a DataTransfer.req from the CSMS but sends a DataTransfer.req to the CSMS
| } | ||
|
|
||
| return response; | ||
| return this->data_transfer->data_transfer_req(request); |
There was a problem hiding this comment.
Same for this one, is it still necessary?
Describe your changes
Issue ticket number and link
Checklist before requesting a review