GVCP support #1357

tigercosmos · 2024-04-12T09:31:26Z

This PR supported the GVCP protocol, including the GVCP request and GVCP acknowledgment.

Also, the PR added the most common commands of GVCP: Discovery command, ForceIP command, Read register Command, and Write register command.

tigercosmos · 2024-10-02T04:38:56Z

@seladb ping.

seladb · 2024-10-02T07:40:25Z

@seladb ping.

Sorry for the delay, I'll review it soon

Common++/header/Logger.h

Packet++/header/ProtocolType.h

Tests/Packet++Test/PacketExamples/gvcp_discovery_ack.pcap

Packet++/header/GvcpLayer.h

seladb · 2024-10-02T07:58:18Z

@tigercosmos I didn't review the entire PR because this comment wasn't addressed yet. Can you please address this and I'll review the rest of the PR?

tigercosmos · 2024-10-02T08:22:47Z

@tigercosmos I didn't review the entire PR because this comment wasn't addressed yet. Can you please address this and I'll review the rest of the PR?

@seladb What do you mean not changed? I have changed all layers. Or do you mean putting the header definition inside the layer? I don't do that because the layer will be too big.

Packet++/header/GvcpLayer.h

seladb · 2024-10-09T07:58:59Z

Packet++/header/GvcpLayer.h

+
+		GvcpCommand getCommand() const
+		{
+			return static_cast<GvcpCommand>(netToHost16(command));


I don't think it's a good practice to put this in the header file, I vote we don't forward declare and move the implementation to the cpp file

seladb · 2024-10-09T08:01:21Z

Packet++/header/GvcpLayer.h

+		GvcpRequestHeader() = default;
+
+		GvcpRequestHeader(GvcpFlag flag, GvcpCommand command, uint16_t dataSize, uint16_t requestId)
+		    : flag(flag), command(hostToNet16(static_cast<uint16_t>(command))), dataSize(hostToNet16(dataSize)),
+		      requestId(hostToNet16(requestId))
+		{}


Why do we need c'tors for a struct? 🤔

What we usually do is declare all properties as public and then we don't need c'tors.
These structs are internal to the GVCP layers classes and shouldn't be exposed outside, so it doesn't matter if their properties are public.

If you want you can move these structs as protected inside of the GVCP layer classes

I added a constructor for it because the byte order will change, and it's better to handle it by the constructor. In addition, GvcpRequestHeader is public in purpose here. Users are free to get the raw header if they want, so there is also a public function getGvcpHeader() in the layers. I don't think we should stop users from getting the header.

I think I mentioned it in some comments, but I'm not sure.... In my opinion there is no need to expose both getGvcpHeader() and getters/setters for each property, that will create a somewhat confusing API.

In older protocols we used to only expose the "raw header" (like getGvcpHeader()), but in newer protocols we usually expose getters and setters which I think provides a cleaner API. We also got feedback from users that they prefer the getters/setters over the "raw header"

Sure, let me not expose it.

they are under internal now

Packet++/header/GvcpLayer.h

seladb · 2024-10-09T08:26:18Z

Packet++/header/GvcpLayer.h

+		 */
+		GvcpCommand getCommand() const
+		{
+			return static_cast<GvcpCommand>(netToHost16(getGvcpHeader()->command));


We can do:

return getGvcpHeader()->getCommand();

seladb · 2024-10-09T08:27:09Z

Packet++/header/GvcpLayer.h

+		 * @param[in] data A pointer to the data including the header and the payload
+		 * @param[in] dataSize The size of the data in bytes
+		 */
+		GvcpAcknowledgeLayer(const uint8_t* data, size_t dataSize);


ditto: why do we need this c'tor if we have the c'tor below?

seladb · 2024-10-09T08:27:38Z

Packet++/header/GvcpLayer.h

+		GvcpAckHeader* getGvcpHeader() const
+		{
+			return reinterpret_cast<GvcpAckHeader*>(m_Data);  // the header is at the beginning of the data
+		}


ditto: this method can be private/protected

seladb · 2024-10-09T08:28:32Z

Packet++/header/GvcpLayer.h

+		 */
+		GvcpResponseStatus getStatus() const
+		{
+			return static_cast<GvcpResponseStatus>((netToHost16(getGvcpHeader()->status)));


This method should handle cases where getGvcpHeader()->status is an unexpected value and return GvcpResponseStatus::Unknown

seladb · 2024-10-09T08:29:00Z

Packet++/header/GvcpLayer.h

+		 */
+		GvcpCommand getCommand() const
+		{
+			return static_cast<GvcpCommand>(netToHost16(getGvcpHeader()->command));


ditto: we can do:

return getGvcpHeader()->getCommand();

seladb · 2024-10-23T05:18:21Z

@tigercosmos a gentle reminder to address the PR comments ☝️

tigercosmos · 2024-10-23T05:51:35Z

@tigercosmos a gentle reminder to address the PR comments ☝️

Thanks. I will continue on this after October. Quite busy these days.

seladb · 2024-10-30T05:35:57Z

Packet++/header/GvcpLayer.h

+
+	using GvcpFlag = uint8_t;  // flag bits are specified by each command
+
+	/// @brief Gvcp command defines the command values and the corresponding acknowledge values


nit: let's use GVCP (capital letters) instead of Gvcp in the doxygen documentation (not in class names etc).
Please search and replace other instances of Gvcp in this file and replace them too

In order to keep the convention we have everywhere else, maybe use this format for doxygen comments?
/** * comment... */

sure fixed.

seladb · 2024-10-30T05:39:48Z

Packet++/header/GvcpLayer.h

+		uint8_t magicNumber = internal::kGvcpMagicNumber;  // always fixed
+		uint8_t flag = 0;  // 0-3 bits are specified by each command, 4-6 bits are reserved, 7 bit is acknowledge
+		uint16_t command = 0;
+		uint16_t dataSize = 0;
+		uint16_t requestId = 0;
+
+		// ------------- methods --------------
+		gvcp_request_header() = default;
+
+		gvcp_request_header(GvcpFlag flag, GvcpCommand command, uint16_t dataSize, uint16_t requestId)
+		    : flag(flag), command(hostToNet16(static_cast<uint16_t>(command))), dataSize(hostToNet16(dataSize)),
+		      requestId(hostToNet16(requestId))
+		{}
+
+		GvcpCommand getCommand() const


If we decide to keep this struct and the other structs public, we should add doxygen documentation for each public member and public method.

If we move them to be private/protected then no doxygen documentation is needed

the struct is now under internal.

seladb · 2024-10-30T05:46:04Z

Packet++/header/GvcpLayer.h

+
+	/// @brief GVCP response status can be returned in an acknowledge message or a GVSP header.
+	/// See more in the spec "Table 19-1: List of Standard Status Codes"
+	enum class GvcpResponseStatus : uint16_t


This enum has doxygen documentation for its values but GvcpCommand doesn't. I think we should be consistent - either we add doxygen to all values in all enums, or we don't...

Some enums in the spec have explanations but some don't. I will add the comments for the enum values as long as the spec has the sentences for the values.

seladb · 2024-10-30T05:47:31Z

Packet++/header/GvcpLayer.h

+		LocalProblem = 0x8008,       // deprecated
+		MsgMismatch = 0x8009,        // deprecated
+		InvalidProtocol = 0x800A,    // deprecated
+		NoMsg = 0x800B,              // deprecated


These // deprecated comments are not in the doxygen format so these values won't appear in the API documentation. We should change them to ///< deprecated.

Same for other // deprecated comments below

seladb · 2024-10-30T05:54:24Z

Packet++/header/GvcpLayer.h

+		GvcpRequestHeader() = default;
+
+		GvcpRequestHeader(GvcpFlag flag, GvcpCommand command, uint16_t dataSize, uint16_t requestId)
+		    : flag(flag), command(hostToNet16(static_cast<uint16_t>(command))), dataSize(hostToNet16(dataSize)),
+		      requestId(hostToNet16(requestId))
+		{}


I think I mentioned it in some comments, but I'm not sure.... In my opinion there is no need to expose both getGvcpHeader() and getters/setters for each property, that will create a somewhat confusing API.

In older protocols we used to only expose the "raw header" (like getGvcpHeader()), but in newer protocols we usually expose getters and setters which I think provides a cleaner API. We also got feedback from users that they prefer the getters/setters over the "raw header"

tigercosmos · 2024-11-13T09:42:35Z

Still WIP, not ready yet.

GVCP init

0184c8c

tigercosmos requested a review from seladb as a code owner April 12, 2024 09:31

tigercosmos marked this pull request as draft April 12, 2024 09:31

tigercosmos and others added 13 commits April 22, 2024 18:07

Merge branch 'dev' of github.com:seladb/PcapPlusPlus into gvcp_0410

4d6ebcb

fix minor build issues

e0a0d85

some fixes

4630418

implement discovery

c55eccf

Merge branch 'dev' into gvcp_0410

c68f703

wip

d5dad27

fix test

f350012

fix

d029b21

add gvcp pcap files

d187ba8

add gvcp hex data

d1cb68c

update test data

36ef8cf

update test

77d058d

fix struct data layout, getter

3784a5f

egecetin added this to the Augest 2024 Release milestone Jun 5, 2024

tigercosmos added 13 commits June 7, 2024 17:50

Merge remote-tracking branch 'upstream/dev' into gvcp_0410

7eaa8a1

format

d974666

add test

711a973

Merge remote-tracking branch 'upstream/dev' into gvcp_0410

73e0ea9

force ip

f8cfda3

remove try block

13ac030

Merge remote-tracking branch 'upstream/master' into gvcp_0410

1d1b095

format + new test

38b96d0

add test

f0a3aca

new test & refactoring

3082195

Merge remote-tracking branch 'upstream/dev' into gvcp_0410

656c690

add GvcpReadRegisterAcknowledge

55d485c

Merge remote-tracking branch 'upstream/dev' into gvcp_0410

567052a

seladb reviewed Oct 2, 2024

View reviewed changes

tigercosmos commented Oct 2, 2024

View reviewed changes

Packet++/header/GvcpLayer.h Outdated Show resolved Hide resolved

tigercosmos added 8 commits October 4, 2024 19:22

move the method to layers

6038329

fix

deedd82

remove author

ea6c556

Merge remote-tracking branch 'upstream/dev' into gvcp_0410

4b7b949

fix

8c307cf

forward declare the function in SystemUtil.h

6758558

rm

aae6efb

add

221fd37

seladb reviewed Oct 9, 2024

View reviewed changes

tigercosmos added 6 commits October 24, 2024 18:49

Merge remote-tracking branch 'upstream/dev' into gvcp_0410

369bbfe

add missing "/// @file"

e9cd0f4

naming issues

5e9877b

remove protect.

722302d

make functions public

fd21069

add some comments

cd8ddf4

seladb reviewed Oct 30, 2024

View reviewed changes

tigercosmos added 5 commits November 13, 2024 18:25

capital GVCP in comments

8300e1c

comments format

e8b67f0

move all GVCP headers under internal

054e73d

fix comments

081ef57

Merge remote-tracking branch 'upstream/dev' into gvcp_0410

9712045

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GVCP support #1357

GVCP support #1357

tigercosmos commented Apr 12, 2024 •

edited

Loading

tigercosmos commented Oct 2, 2024

seladb commented Oct 2, 2024

seladb commented Oct 2, 2024

tigercosmos commented Oct 2, 2024

seladb Oct 9, 2024

seladb Oct 9, 2024

tigercosmos Oct 24, 2024

seladb Oct 30, 2024

tigercosmos Nov 13, 2024

tigercosmos Nov 13, 2024

seladb Oct 9, 2024

seladb Oct 9, 2024

seladb Oct 9, 2024

seladb Oct 9, 2024

seladb Oct 9, 2024

seladb commented Oct 23, 2024

tigercosmos commented Oct 23, 2024

seladb Oct 30, 2024

tigercosmos Nov 13, 2024

seladb Oct 30, 2024 •

edited

Loading

tigercosmos Nov 13, 2024

seladb Oct 30, 2024

tigercosmos Oct 31, 2024

seladb Oct 30, 2024

tigercosmos Nov 13, 2024

seladb Oct 30, 2024

tigercosmos commented Nov 13, 2024


		using GvcpFlag = uint8_t; // flag bits are specified by each command

		/// @brief Gvcp command defines the command values and the corresponding acknowledge values

GVCP support #1357

Are you sure you want to change the base?

GVCP support #1357

Conversation

tigercosmos commented Apr 12, 2024 • edited Loading

tigercosmos commented Oct 2, 2024

seladb commented Oct 2, 2024

seladb commented Oct 2, 2024

tigercosmos commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seladb commented Oct 23, 2024

tigercosmos commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seladb Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Nov 13, 2024

tigercosmos commented Apr 12, 2024 •

edited

Loading

seladb Oct 30, 2024 •

edited

Loading