-
Notifications
You must be signed in to change notification settings - Fork 714
Add Layer Build and Validation for DoIP (Diagnostic over IP) Support #1655
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
base: dev
Are you sure you want to change the base?
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 |
Tests/Packet++Test/PacketExamples/DoIpAliveCheckRequestPacket.dat
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1655 +/- ##
==========================================
+ Coverage 83.10% 83.55% +0.44%
==========================================
Files 283 287 +4
Lines 48929 50953 +2024
Branches 10303 10797 +494
==========================================
+ Hits 40664 42573 +1909
- Misses 7113 7587 +474
+ Partials 1152 793 -359
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if i am understanding correctly the sequence is this?
- User somehow receives a
DoIPLayer
from 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you are absolutely correct.
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.
Sure, that works fine. 👍
Co-authored-by: Dimitar Krastev <dimi1010100@gmail.com>
…Ack and diagnosticNac
@seladb, could you please have a look, I've made serval changes in this current version and layer now is more aligned with other layer implementation, you can start reviewing this PR, |
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.
@raissi-oussema I mostly reviewed the base class DoIpLayer
but since there are enough comments, I suggest you address those before I review the rest of the classes and files
@seladb , could you please have a look and suggest what we can improve more |
@seladb , could you have a look, I've made suggested changes. |
std::string DoIpVehicleIdentificationRequestVIN::getSummary() const | ||
{ | ||
std::ostringstream oss; | ||
oss << "VIN: " << std::string(reinterpret_cast<const char*>(getVIN().data()), DOIP_VIN_LEN) << "\n"; |
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 VIN is always a string, should getVIN()
return a string and setVIN()
accept a string? 🤔
Same for the constructor - should it accept a string VIN?
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 will keep it like this as disscussed in this comment .
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 VIN can include non-printable characters, we shouldn't convert it to string... We should either print the hex representation of it using pcpp::byteArrayToHexString()
or not print it at all
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.
@raissi-oussema I'm not sure you saw this comment ☝️
@seladb , Could you check if there are any areas left to improve or optimize further? |
std::string DoIpVehicleIdentificationRequestVIN::getSummary() const | ||
{ | ||
std::ostringstream oss; | ||
oss << "VIN: " << std::string(reinterpret_cast<const char*>(getVIN().data()), DOIP_VIN_LEN) << "\n"; |
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 VIN can include non-printable characters, we shouldn't convert it to string... We should either print the hex representation of it using pcpp::byteArrayToHexString()
or not print it at all
std::string DoIpVehicleIdentificationRequestVIN::getSummary() const | ||
{ | ||
std::ostringstream oss; | ||
oss << "VIN: " << std::string(reinterpret_cast<const char*>(getVIN().data()), DOIP_VIN_LEN) << "\n"; |
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.
@raissi-oussema I'm not sure you saw this comment ☝️
PTF_ASSERT_EQUAL(doipLayer.getProtocolVersion(), pcpp::DoIpProtocolVersion::ISO13400_2012, enumclass); | ||
doipLayer.setProtocolVersion(0x55); | ||
PTF_ASSERT_EQUAL(doipLayer.getProtocolVersion(), pcpp::DoIpProtocolVersion::UNKNOWN, enumclass); | ||
PTF_ASSERT_EQUAL(doipLayer.getProtocolVersionAsStr(), "Unknown Protocol Version"); |
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.
Can we add tests for this method in some of the other tests?
unsigned char bytes[] = { 0x2, 0xfd, 0x0, 0x5, 0x0, 0x0, 0x0, 0xb, 0xe, 0x80, | ||
0x0, 0x1, 0x2, 0x3, 0x4, 0x05, 0x05, 0x05, 0x05 }; | ||
unsigned char bytesWithoutOem[] = { 0x2, 0xfd, 0x0, 0x5, 0x0, 0x0, 0x0, 0x7, 0xe, 0x80, 0x0, 0x1, 0x2, 0x3, 0x4 }; |
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.
Does it make sense to add these packets as .dat
files and also to doipPackets.pcapng
?
If we do that we can compare the created packet to the one in the .dat
file.
Let me know what you think.
Same for the other ...Creation
tests
std::array<uint8_t, 6> eid{ 0x42, 0x41, 0x55, 0x4e, 0x45, 0x45 }; | ||
PTF_ASSERT_VECTORS_EQUAL(doipLayer->getEID(), eid); | ||
|
||
} // VehicleIdentificationWithEIDacketParsing |
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 should match the test name: DoIpVehicleIdentificationRequestWEIDPacketParsing
Also - let's be consistent and add this comment to all tests?
} // DoIpVehicleIdentificationRequestVINPacketCreation | ||
|
||
// DoIpVehicleAnnouncementPacketParsing | ||
PTF_TEST_CASE(DoIpVehicleAnnouncementPacketParsing) |
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.
Let's add a test for getFurtherActionRequired()
?
} // DoIpVehicleAnnouncementPacketParsing | ||
|
||
// DoIpVehicleAnnouncementPacketCreation | ||
PTF_TEST_CASE(DoIpVehicleAnnouncementPacketCreation) |
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.
In this test (and maybe other ...Creation
tests) - can't we use the corresponding .dat
file, meaning build a packet similar to the one in the .dat
file and then compare the bytes of the created packet to the original packet?
} // DoIpAliveCheckResponsePacketCreation | ||
|
||
// DoIpPowerModeResponsePacketParsing | ||
PTF_TEST_CASE(DoIpPowerModeResponsePacketParsing) |
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.
Let's rename the test to DoIpDiagnosticPowerModeResponsePacketParsing
to match the layer name?
doIpPacket.computeCalculateFields(); | ||
|
||
unsigned char bytes[] = { 0x2, 0xfd, 0x40, 0x4, 0x0, 0x0, 0x0, 0x1, 0x1 }; | ||
pcpp::DoIpDiagnosticPowerModeResponse data(pcpp::DoIpDiagnosticPowerModeCodes::READY); |
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: let's call this variable powerModeResposne
or something along these lines?
Same in other tests like DoIpDiagnosticAckMessagePacketCreation
} // DoIpDiagnosticNackMessagePacketCreation | ||
|
||
// DoIpPowerModeRequestPacketParsing | ||
PTF_TEST_CASE(DoIpPowerModeRequestPacketParsing) |
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.
ditto: let's rename the test to DoIpDiagnosticPowerModeRequestPacketParsing
?
// DoIpInvalidPayloadTypePacketParsing | ||
PTF_TEST_CASE(DoIpInvalidPayloadTypePacketParsing) | ||
{ | ||
timeval time; | ||
gettimeofday(&time, nullptr); | ||
|
||
READ_FILE_AND_CREATE_PACKET(1, "PacketExamples/DoIpInvalidPayloadTypePacket.dat"); | ||
|
||
pcpp::Packet InvalidPayloadTypePacket(&rawPacket1); | ||
PTF_ASSERT_FALSE(InvalidPayloadTypePacket.isPacketOfType(pcpp::DOIP)); | ||
|
||
} // DoIpInvalidPayloadTypePacketParsing | ||
|
||
// DoIpInvalidPayloadTypePacketPacketParsing | ||
PTF_TEST_CASE(DoIpInvalidPayloadLenPacketParsing) | ||
{ | ||
timeval time; | ||
gettimeofday(&time, nullptr); | ||
|
||
READ_FILE_AND_CREATE_PACKET(1, "PacketExamples/DoIpWrongLengthRoutingActivationRequestPacket.dat"); | ||
|
||
pcpp::Packet InvalidPayloadLenPacket(&rawPacket1); | ||
PTF_ASSERT_FALSE(InvalidPayloadLenPacket.isPacketOfType(pcpp::DOIP)); | ||
|
||
} // DoIpInvalidPayloadTypePacketParsing | ||
|
||
// DoIpInvalidProtocolVersion | ||
PTF_TEST_CASE(DoIpInvalidProtocolVersionPacketParsing) | ||
{ | ||
timeval time; | ||
gettimeofday(&time, nullptr); | ||
|
||
READ_FILE_AND_CREATE_PACKET(1, "PacketExamples/DoIpInvalidProtocolVersionPacket.dat"); | ||
|
||
pcpp::Packet InvalidPayloadLenPacket(&rawPacket1); | ||
PTF_ASSERT_FALSE(InvalidPayloadLenPacket.isPacketOfType(pcpp::DOIP)); | ||
|
||
} // DoIpInvalidProtocolVersion |
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 we can merge them into one test DoIpInvalidPackets
?
@raissi-oussema please also note this comment: #1655 (comment) |
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