Is this for another PR? If so, should the remaining data be parsed as a generic payload layer for now?

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.

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;
			}
		}`

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?

@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 ?

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).

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 ?

Ok, if i am understanding correctly the sequence is this?

1. User somehow receives a DoIPLayer from a Packet
2. User checks the payload type (via DoIPLayer::getPayloadType())
3. 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?

yes you are absolutely correct.

Sure, that works fine. 👍

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?

we will keep it like this as disscussed in this comment .

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

@raissi-oussema I'm not sure you saw this comment ☝️

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

if currentDiagnosticDataLen > 0 we're shortening the layer and then extending it again. Insead we can calculate the length and do a single change:

auto layerExtensionLen = data.size() - currentDiagnosticDataLen;
if (layerExtensionLen > 0)
{
	extendLayer(DIAGNOSTIC_DATA_OFFSET + currentDiagnosticDataLen, layerExtensionLen );
}
else if (layerExtensionLen < 0)
{
	shortenLayer(DIAGNOSTIC_DATA_OFFSET, layerExtensionLen );
}

I believe the logic here might be incorrect. When subtracting two size_t values, a negative result causes unsigned underflow and produces a very large number. If layerExtensionLen is negative, we likely need to shrink the layer starting from DIAGNOSTIC_DATA_OFFSET + data.size(), using the absolute value of the difference (i.e., -1 * layerExtensionLen).
code will looks like this :

void DoIpDiagnosticMessage::setDiagnosticData(const std::vector<uint8_t>& data)
	{
		const size_t newPayloadLength = DOIP_SOURCE_ADDRESS_LEN + DOIP_TARGET_ADDRESS_LEN + data.size();
		const size_t currentDiagnosticDataLen = m_DataLen - DIAGNOSTIC_DATA_OFFSET;
		setPayloadLength(newPayloadLength);

		ptrdiff_t layerExtensionLen =
		    static_cast<ptrdiff_t>(data.size()) - static_cast<ptrdiff_t>(currentDiagnosticDataLen);
		if (layerExtensionLen > 0)
		{
			extendLayer(DIAGNOSTIC_DATA_OFFSET + currentDiagnosticDataLen, layerExtensionLen);
		}
		else if (layerExtensionLen < 0)
		{
			shortenLayer(DIAGNOSTIC_DATA_OFFSET + data.size(), (-1 * layerExtensionLen));
		}
		memcpy((m_Data + DIAGNOSTIC_DATA_OFFSET), data.data(), data.size());
	}

You're right! Another option is to static_cast both sizes to int and then we can get a negative number. I think it's a bit safer than extracting pointers 🤔

This works correctly since data.size() is implicitly convertible to int meaning it's guaranteed to be less than INT_MAX. In the automotive context, it's unlikely that diagnostic data will ever exceed 2,147,483,647 bytes on a 32-bit system.

That said, relying on this assumption can be risky in edge cases (e.g., malformed input or fuzzing). I used ptrdiff_t to ensure we avoid potential overflows or undefined behavior.

If we’re confident this situation will never arise in production, I’m open to simplifying it.

I agree, using ptrdiff_t is a good option here 👍

ditto: instead of shortening and then extending we can calculate the size and decide which action is needed

@raissi-oussema I'm not sure you saw this comment ☝️

Since this is an abstract layer, maybe we can make these constructors protected?

ditto: this c'tor can also be protected

This file contains 16 packets, but there are 20 .dat files... are there packets missing in the pcap file?

I'm not sure these checks are needed, we only need to assert that the packet is of type DoIP.

Same for the other tests below

Let's be consistent and use either PTF_ASSERT_VECTORS_EQUAL or PTF_ASSERT_BUF_COMPARE in both of these asserts?

I don't think we need to set sourceAddress and activateType both in the c'tor and again here. We can set the expected values in the c'tor, and test the setters later in this test.

Same in the other tests below

These getters are already tested in DoIpRoutingActivationRequestPacketParsing, no?