Add Layer Build and Validation for DoIP (Diagnostic over IP) Support #1655
Conversation
|
As per the contributing guidelines, please retarget the PR to the |
|
Observed several issues in the CI pipelines, likely due to missing definitions for |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1655 +/- ##
==========================================
+ Coverage 82.88% 83.43% +0.54%
==========================================
Files 292 295 +3
Lines 51834 53814 +1980
Branches 11293 11863 +570
==========================================
+ Hits 42965 44902 +1937
- Misses 7671 8093 +422
+ Partials 1198 819 -379
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@raissi-oussema are you planning to continue working on this PR? |
|
Hi, I was engaged with other tasks, but I plan to get back to this PR soon. Thanks for your understanding! |
.improve maps searchs for doipEnumsToStrings .cover more uses cases based on codecov feedback
|
Design suggestions or code improvements are always welcome and greatly appreciated. |
|
@raissi-oussema to make it easier to review, do you think you can add some documentation on the DoIP protocol to the PR body? It'd mostly be helpful to get more details on the header structure and different possible message |
|
@seladb I need support for CI pipelines, I can't figure out why are they still failing. And a clear documentation was successfully added to PR body to make it easier for you to start the code review. |
Doxigen pipeline: XDP pipeline: VS pipeline: |
What could be the problem for dioxygen pipeline, doipLayer.h is well documented [line 42] ? |
Packet++/header/DoIpLayer.h
Outdated
| // implement abstract methods | ||
|
|
||
| /** | ||
| * TODO, parse UDS layer |
There was a problem hiding this comment.
Is this for another PR? If so, should the remaining data be parsed as a generic payload layer for now?
There was a problem hiding this comment.
m_NextLayer is intended to be the UDS layer, which has not been implemented yet. In the future, as more knowledge is gained, either I or another contributor may add this functionality. For now, I suggest parsing it as a generic layer, as you mentioned.
PS: the nextLayer will be parsed only when the payloadType is 0x8001.
There was a problem hiding this comment.
your feedback is highly appertiated, what do you think about adding this snippet of code:
void parseNextLayer() override
{
if (getPayloadType() == DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE)
{
size_t headerLen = sizeof(doiphdr);
if (m_DataLen <= headerLen + 2 /*source address size*/ + 2 /*target address size*/)
return;
uint8_t* payload = m_Data + (headerLen + 2 + 2);
size_t payloadLen = m_DataLen - (headerLen + 2 + 2);
m_NextLayer = new PayloadLayer(payload, payloadLen, this, m_Packet);
return;
}
}`
There was a problem hiding this comment.
your feedback is highly appertiated, what do you think about adding this snippet of code:
void parseNextLayer() override { if (getPayloadType() == DoIpPayloadTypes::DIAGNOSTIC_MESSAGE_TYPE) { size_t headerLen = sizeof(doiphdr); if (m_DataLen <= headerLen + 2 /*source address size*/ + 2 /*target address size*/) return; uint8_t* payload = m_Data + (headerLen + 2 + 2); size_t payloadLen = m_DataLen - (headerLen + 2 + 2); m_NextLayer = new PayloadLayer(payload, payloadLen, this, m_Packet); return; } }`
Seems good. Can the source address and target address used in that type of message be accessed from this layer? Since we are excluding it from the generic payload.
A minor tip, having headerLen be marked as constexpr might allow some compiler optimizations (such as the arithmetic using it + a literal being computed during compilation and hardcoded if possible).
Also why the return statement that is at the end of the block anyway?
There was a problem hiding this comment.
@Dimi1010 , this approach appears both safer and cleaner. It allows us to construct a DiagnosticMessage directly from the current layer, providing direct access to the diagnostic data. Using this data, we can then build a generic PayloadLayer.
void parseNextLayer() override
{
DiagnosticMessageData diagnosticMessage;
if (diagnosticMessage.buildFromLayer(this))
{
m_NextLayer = new PayloadLayer(diagnosticMessage.diagnosticData.data(),
diagnosticMessage.diagnosticData.size(), this, m_Packet);
}
}
buildFromLayer safely parses the current layer and verifies whether it represents a valid diagnostic message.
what do you think ?
There was a problem hiding this comment.
Sure, but my question stands. DiagnosticMessageData has two other members (sourceAddress and targetAddress) which at the moment I don't see how the user can access them easily. They have neither accessors in the current DoIPLayer or are included as part of the UDSLayer (currently PayloadLayer).
There was a problem hiding this comment.
sourceAddress and targetAddress are public members of DiagnosticMessageData class just like diagnosticData and they are not part of the UDS layer:
uint16_t sourceAddress; /**< Source address of the message. */
uint16_t targetAddress; /**< Target address for the diagnostic message. */
std::vector<uint8_t> diagnosticData; /**< Diagnostic message data with dynamic length. */
the user can access these fields by the method buildFromLayer .
I've made this dummy function just to show how to access to these fields :
void DoIpLayer::resolveDiagMessageFields()
{
DiagnosticMessageData diagnosticMessage;
if (diagnosticMessage.buildFromLayer(this))
{
uint16_t srcAddr = diagnosticMessage.sourceAddress;
uint16_t targetAddr = diagnosticMessage.targetAddress;
std::vector<uint8_t> diagData = diagnosticMessage.diagnosticData;
}
}
Do you think this implementation need more improvement ?
There was a problem hiding this comment.
Ok, if i am understanding correctly the sequence is this?
- User somehow receives a
DoIPLayerfrom aPacket - User checks the payload type (via
DoIPLayer::getPayloadType()) - Depending on the payload type user uses
T::buildFromLayer(DoIPLayer)(T being the corresponding message struct) to populate the data from the layer into the struct.
Am I understanding the flow correctly?
There was a problem hiding this comment.
yes you are absolutely correct.
There was a problem hiding this comment.
Sure, that works fine. 👍
Co-authored-by: Dimitar Krastev <dimi1010100@gmail.com>
|
@raissi-oussema please also note this comment: #1655 (comment) |
|
@raissi-oussema I think this PR is almost ready to merge. Can you address the remaining comments? |
I was tied up with other tasks, but I’ll take care of the remaining work over the next two days. Thanks for your patience! |
|
@seladb, could you have a look, I've addressed the last comments and I made some improvements. |
There was a problem hiding this comment.
@raissi-oussema just 2 last comments and I think we can merge this PR!
|
@raissi-oussema thank you so much for working on this PR! 🙏 |
5 participants
DoIP Protocol Overview
The Diagnostic over IP (DoIP) protocol is used in automotive diagnostic systems to facilitate communication between diagnostic tools and ECUs (Electronic Control Units) over IP-based networks. It enables remote diagnostics, configuration, and software updates over Ethernet, offering an efficient and scalable solution for modern vehicles.
Header Structure (8 bytes)
Pyload types / code / structure