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 ☝️

@seladb , the same logic is discussed in this comment, and I think we agree to keep it printing the string representation.

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 ☝️

Can we add tests for this method in some of the other tests?

We never check for a valid version of getProtocolVersionAsStr(), all the tests only check "Unknown Protocol Version"

This should match the test name: DoIpVehicleIdentificationRequestWEIDPacketParsing

Also - let's be consistent and add this comment to all tests?

There is still a mismatch in 2 tests:

• DoIpVehicleIdentificationRequestWithVINPacketParsing --> the comment in the end says DoIpVehicleIdentificationRequestVINPacketParsing
• DoIpVehicleIdentificationRequestWithVINPacketCreation --> the comment in the end says DoIpVehicleIdentificationRequestVINPacketCreation

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?

@raissi-oussema you didn't fix this comment ☝️

nit: let's rename this variable to start with lower case powerModeResponsePacket?

ditto: can we rename these variable to start with lower case?