#1754 Add Modbus Support by yahyayozo · Pull Request #1823 · seladb/PcapPlusPlus

yahyayozo · 2025-05-18T18:15:25Z

Add ModbusLayer first implementation with header parsing only (no test included)

codecov · 2025-05-18T19:05:44Z

Codecov Report

❌ Patch coverage is 91.86992% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.47%. Comparing base (699f7b0) to head (e3461d0).
⚠️ Report is 6 commits behind head on dev.

Files with missing lines	Patch %	Lines
Packet++/src/ModbusLayer.cpp	88.05%	8 Missing ⚠️
Packet++/header/ModbusLayer.h	90.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1823      +/-   ##
==========================================
- Coverage   83.47%   83.47%   -0.01%     
==========================================
  Files         298      304       +6     
  Lines       53945    54152     +207     
  Branches    11705    12013     +308     
==========================================
+ Hits        45029    45201     +172     
- Misses       7709     7735      +26     
- Partials     1207     1216       +9

Flag	Coverage Δ
alpine320	`75.45% <93.33%> (+0.02%)`	⬆️
fedora42	`75.60% <93.44%> (+0.06%)`	⬆️
macos-13	`81.67% <89.10%> (+<0.01%)`	⬆️
macos-14	`81.67% <89.10%> (+<0.01%)`	⬆️
macos-15	`81.67% <89.10%> (+<0.01%)`	⬆️
mingw32	`70.29% <78.72%> (-0.01%)`	⬇️
mingw64	`70.27% <78.72%> (-0.02%)`	⬇️
rhel94	`75.30% <93.44%> (+0.02%)`	⬆️
ubuntu2004	`59.24% <76.56%> (+0.03%)`	⬆️
ubuntu2004-zstd	`59.36% <76.56%> (+0.05%)`	⬆️
ubuntu2204	`75.23% <93.44%> (-0.01%)`	⬇️
ubuntu2204-icpx	`60.93% <70.58%> (+0.04%)`	⬆️
ubuntu2404	`75.50% <93.44%> (+0.03%)`	⬆️
ubuntu2404-arm64	`75.47% <93.44%> (+0.02%)`	⬆️
unittest	`83.47% <91.86%> (-0.01%)`	⬇️
windows-2022	`85.49% <86.95%> (+0.01%)`	⬆️
windows-2025	`85.51% <86.95%> (+0.01%)`	⬆️
winpcap	`85.51% <86.95%> (+0.01%)`	⬆️
xdp	`53.09% <93.44%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

yahyayozo · 2025-05-21T20:14:44Z

@seladb I've prepared only 1 constructor which initiates the layer with default fields values
Can you please mention what are the other constructors I need to implement? also please mention if I need to follow on other protocols layers?

seladb · 2025-05-22T06:53:53Z

@yahyayozo we usually have at least 2 c'tors, sometimes more:

As part of the packet parsing flow. For example:

PcapPlusPlus/Packet++/header/MplsLayer.h

Line 37 in a49a79e

MplsLayer(uint8_t* data, size_t dataLen, Layer* prevLayer, Packet* packet)

which is used here (and in some other places also):

PcapPlusPlus/Packet++/src/VlanLayer.cpp

Line 101 in a49a79e

m_NextLayer = new MplsLayer(payload, payloadLen, this, m_Packet);
When the user wants to create a new layer of this type. This is the c'tor you already implemented, but usually we don't implement (only) c'tors with zero arguments. We tend to implement c'tors that allow the user to build the layer. In your case, I'd add arguments like transactionId, unitId and functionCode

…sPlus into feature/modbus

yahyayozo · 2025-05-24T10:48:32Z

Hi @seladb can you please guide me briefly on how to add tests for my ModbusLayer for parsing/crafting headers? As I want to pass to function types request/response after testing the header part.

seladb · 2025-05-25T07:11:43Z

@yahyayozo sure! You can learn about tests here: https://pcapplusplus.github.io/docs/tests

At a high level:

You should add your tests under Tests/Packet++Test/Tests (don't forget to add your .cpp file to CMakeFiles.txt)
The tests use a homegrown testing framework. A test case is PTF_TEST_CASE. You can write 3 of those: for parsing, crafting, and editing
You can find the possible assertions here, the most common one is PTF_ASSERT_EQUAL(actual, expected)
You need to add your test cases to https://github.com/seladb/PcapPlusPlus/blob/master/Tests/Packet%2B%2BTest/main.cpp and add a PTF_RUN_TEST for each test case
When you build the tests, you should run them from the Tests/Packet++Test directory, like this: Bin/Packet++Test -t <YOUR_TEST_CASE>

Please look at similar tests that were written for other layers and follow the same patterns.

Please let me know if you have any questions.

yahyayozo · 2025-05-28T21:59:03Z

@seladb I'll add tests for the headers crafting/parsing implementation and then you can a review before we move to adding the different PDU types

yahyayozo · 2025-06-02T20:08:24Z

@seladb @Dimi1010 I've started adding some tests, but I have a question regarding the ".dat" files; how can I generate examples for some Modbus frames?

seladb · 2025-06-02T21:58:05Z

@seladb @Dimi1010 I've started adding some tests, but I have a question regarding the ".dat" files; how can I generate examples for some Modbus frames?

If you open the file in Wireshark, you can choose a packet and right click -> Copy -> ... as a Hex Stream:

… instead of raw data

seladb · 2025-08-12T02:24:35Z

@yahyayozo the tests fail in CI...

…sPlus into feature/modbus

yahyayozo · 2025-08-12T22:19:28Z

@seladb should be fixed now

seladb · 2025-08-15T08:13:10Z

Packet++/src/ModbusLayer.cpp

+{
+	ModbusLayer::ModbusLayer(uint16_t transactionId, uint8_t unitId, ModbusLayer::ModbusFunctionCode functionCode)
+	{
+		const int16_t pduSize = getFunctionDataSize(functionCode);


I can see that getFunctionDataSize() currently supports only one function code, which is ModbusFunctionCode::READ_INPUT_REGISTERS. In that case, maybe we can remove the functionCode parameter, remove the getFunctionDataSize() method, and set the function code in this constructor to be ModbusFunctionCode::READ_INPUT_REGISTERS

This comment wasn't addressed

Packet++/src/ModbusLayer.cpp

Packet++/header/ModbusLayer.h

seladb · 2025-08-15T08:16:53Z

Packet++/header/ModbusLayer.h

+		/// @struct ModbusReadInputRegisters
+		/// Represents a Modbus request to read input registers.
+		struct ModbusReadInputRegisters
+		{
+			uint16_t startingAddress;  ///< Starting address of the input registers to read
+			uint16_t quantity;         ///< Number of input registers to read
+		};


This struct is only used in getFunctionDataSize(). If we remove it we can remove this struct as well

Packet++/src/ModbusLayer.cpp

Tests/Packet++Test/Tests/ModbusTests.cpp

Packet++/src/ModbusLayer.cpp

Packet++/header/ModbusLayer.h

seladb · 2025-08-20T05:42:59Z

Packet++/src/ModbusLayer.cpp

+{
+	ModbusLayer::ModbusLayer(uint16_t transactionId, uint8_t unitId, ModbusLayer::ModbusFunctionCode functionCode)
+	{
+		const int16_t pduSize = getFunctionDataSize(functionCode);


This comment wasn't addressed

Packet++/header/ModbusLayer.h

seladb · 2025-08-20T05:48:05Z

@yahyayozo please also see older comments:

seladb · 2025-08-23T20:28:26Z

@yahyayozo I think the only comments left are:

Once you address those I think we can merge this PR

yahyayozo · 2025-08-25T11:46:55Z

@seladb all the comments shall be addressed now
I kept the ModbusReadInputRegisters struct because I use in the constructor and also to keep it as an example for the future implementation of other functions Codes

yahyayozo · 2025-08-25T14:57:28Z

@seladb can you please check why it shows forbid submodules in the failed pre-commit?

Dimi1010 · 2025-08-25T16:10:04Z

@seladb can you please check why it shows forbid submodules in the failed pre-commit?

a7b926d this commit adds deps/benchmark-src to the repo. I think this is a mistake.

yahyayozo · 2025-08-25T16:34:26Z

@seladb @Dimi1010 I removed it
I don't know how it was added as a submodule!!

seladb · 2025-08-26T06:48:21Z

@yahyayozo thank you so much for working on this and contributing to PcapPlusPlus, it is much appreciated! 🙏

yahyayozo · 2025-08-26T18:27:45Z

@seladb so, the other PDUs types will added in another PR?

Dimi1010 · 2025-08-26T18:30:31Z

Yes, it's easier to review that way as it results in smaller differential changes in PRs.

seladb · 2025-08-26T22:17:40Z

Yes, it's easier to review that way as it results in smaller differential changes in PRs.

Yes I agree. Smaller PRs are always better and easier to review

yahyayozo added 2 commits May 18, 2025 19:09

Add ModbusLayer first implementation with header parsing only

a36e594

fix static_assert

88f69d4

yahyayozo requested a review from seladb as a code owner May 18, 2025 18:15

Merge branch 'dev' into feature/modbus

c257494

yahyayozo marked this pull request as draft May 18, 2025 18:16

Dimi1010 linked an issue May 19, 2025 that may be closed by this pull request

Add Modbus Protocol Support #1813

Closed

Dimi1010 added the enhancement label May 20, 2025

Fix formatting in ModbusLayer header file

348afc6

yahyayozo marked this pull request as ready for review May 21, 2025 20:13

yahyayozo added 2 commits May 21, 2025 21:45

Add setter methods for MODBUS header fields in ModbusLayer

1f65c24

Merge branch 'dev' into feature/modbus

4159c6c

yahyayozo added 6 commits May 23, 2025 19:02

Refactor ModbusLayer class documentation and fix syntax error

58c18ec

Merge branch 'feature/modbus' of https://github.com/yahyayozo/PcapPlu…

d50db3b

…sPlus into feature/modbus

Fix endianess handling in ModbusLayer getters and setters

cfcbc69

Add const qualifier to ModbusLayer getter methods

28dd176

Add constructors to ModbusLayer for initializing from user inputs

34d65a2

Add Modbus protocol constant to ProtocolType

74ab79d

Add const qualifier to ModbusLayer getter methods in .h file

166c15c

yahyayozo marked this pull request as draft May 28, 2025 22:00

yahyayozo added 2 commits May 29, 2025 22:20

Add ModbusLayer and corresponding tests to Packet++Test

3624286

Fix length assignment in ModbusLayer constructor to use htobe16

bcd14b5

Refactor ModbusLayerParsingTest to create ModbusLayer from a dat file…

9f4ebbd

… instead of raw data

yahyayozo added 4 commits August 12, 2025 23:06

refactor

18a80df

Merge branch 'dev' into feature/modbus

3c746e9

fix: wrong header length in modbus

ec3e582

Merge branch 'feature/modbus' of https://github.com/yahyayozo/PcapPlu…

4a8f115

…sPlus into feature/modbus

seladb reviewed Aug 15, 2025

View reviewed changes

yahyayozo added 5 commits August 16, 2025 15:22

some refactoring

c50f58b

Merge branch 'dev' into feature/modbus

ea86788

formatting

0cd7502

formatting

fc595a7

style

951fe03

seladb reviewed Aug 20, 2025

View reviewed changes

refactor: adjust comment style and enum case

93fb5e7

yahyayozo added 2 commits August 25, 2025 12:40

refactor

a7b926d

refactor: fixing comments

8bc7c47

fix: remove _deps

e3461d0

seladb approved these changes Aug 26, 2025

View reviewed changes

seladb merged commit 8e44878 into seladb:dev Aug 26, 2025
42 checks passed

seladb mentioned this pull request Aug 26, 2025

Add Modbus Protocol Support #1813

Closed

Conversation

yahyayozo commented May 18, 2025

Uh oh!

codecov bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yahyayozo commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seladb commented May 22, 2025

Uh oh!

yahyayozo commented May 24, 2025

Uh oh!

seladb commented May 25, 2025

Uh oh!

yahyayozo commented May 28, 2025

Uh oh!

yahyayozo commented Jun 2, 2025

Uh oh!

seladb commented Jun 2, 2025

Uh oh!

seladb commented Aug 12, 2025

Uh oh!

yahyayozo commented Aug 12, 2025

Uh oh!

seladb Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

seladb Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seladb Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seladb Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seladb commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seladb commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yahyayozo commented Aug 25, 2025

Uh oh!

yahyayozo commented Aug 25, 2025

Uh oh!

Dimi1010 commented Aug 25, 2025

Uh oh!

yahyayozo commented Aug 25, 2025

Uh oh!

Uh oh!

seladb commented Aug 26, 2025

Uh oh!

yahyayozo commented Aug 26, 2025

Uh oh!

Dimi1010 commented Aug 26, 2025

Uh oh!

seladb commented Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented May 18, 2025 •

edited

Loading

yahyayozo commented May 21, 2025 •

edited

Loading

seladb commented Aug 20, 2025 •

edited

Loading

seladb commented Aug 23, 2025 •

edited

Loading