GVCP support #1357
GVCP support #1357
Conversation
Packet++/header/GvcpLayer.h
Outdated
|
|
||
| GvcpCommand getCommand() const | ||
| { | ||
| return static_cast<GvcpCommand>(netToHost16(command)); |
There was a problem hiding this comment.
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
Packet++/header/GvcpLayer.h
Outdated
| 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)) | ||
| {} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Sure, let me not expose it.
There was a problem hiding this comment.
they are under internal now
Packet++/header/GvcpLayer.h
Outdated
| */ | ||
| GvcpCommand getCommand() const | ||
| { | ||
| return static_cast<GvcpCommand>(netToHost16(getGvcpHeader()->command)); |
There was a problem hiding this comment.
We can do:
return getGvcpHeader()->getCommand();| * @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); |
There was a problem hiding this comment.
ditto: why do we need this c'tor if we have the c'tor below?
Packet++/header/GvcpLayer.h
Outdated
| GvcpAckHeader* getGvcpHeader() const | ||
| { | ||
| return reinterpret_cast<GvcpAckHeader*>(m_Data); // the header is at the beginning of the data | ||
| } |
There was a problem hiding this comment.
ditto: this method can be private/protected
Packet++/header/GvcpLayer.h
Outdated
| */ | ||
| GvcpResponseStatus getStatus() const | ||
| { | ||
| return static_cast<GvcpResponseStatus>((netToHost16(getGvcpHeader()->status))); |
There was a problem hiding this comment.
This method should handle cases where getGvcpHeader()->status is an unexpected value and return GvcpResponseStatus::Unknown
Packet++/header/GvcpLayer.h
Outdated
| */ | ||
| GvcpCommand getCommand() const | ||
| { | ||
| return static_cast<GvcpCommand>(netToHost16(getGvcpHeader()->command)); |
There was a problem hiding this comment.
ditto: we can do:
return getGvcpHeader()->getCommand();
|
@tigercosmos a gentle reminder to address the PR comments ☝️ |
Thanks. I will continue on this after October. Quite busy these days. |
Packet++/header/GvcpLayer.h
Outdated
|
|
||
| using GvcpFlag = uint8_t; // flag bits are specified by each command | ||
|
|
||
| /// @brief Gvcp command defines the command values and the corresponding acknowledge values |
There was a problem hiding this comment.
- 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... */
Packet++/header/GvcpLayer.h
Outdated
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
the struct is now under internal.
|
|
||
| /// @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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Packet++/header/GvcpLayer.h
Outdated
| LocalProblem = 0x8008, // deprecated | ||
| MsgMismatch = 0x8009, // deprecated | ||
| InvalidProtocol = 0x800A, // deprecated | ||
| NoMsg = 0x800B, // deprecated |
There was a problem hiding this comment.
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
Packet++/header/GvcpLayer.h
Outdated
| 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)) | ||
| {} |
There was a problem hiding this comment.
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"
|
Still WIP, not ready yet. |
@tigercosmos maybe you can move the PR to draft until it's ready? |
|
Ops, I didn't notice this. Changed. |
| /* | ||
| * GVCP protocol | ||
| */ | ||
| const ProtocolType GVCP = 57; |
There was a problem hiding this comment.
Protocol number 57 is already taken by GTPv2.
| { | ||
| namespace internal | ||
| { | ||
| std::unordered_set<GvcpCommand> gvcpCommandSet = { |
There was a problem hiding this comment.
As far as I could see, gvcpCommandSet is only used in the current file.
It would be better to change the symbol to use internal linkage, rather than external, as it is not declared in GvcpLayer.h.
There was a problem hiding this comment.
nit: Also maybe have the set as const?
| pcpp::MacAddress getMacAddress() const | ||
| { | ||
| auto body = this->getGvcpDiscoveryBody(); | ||
| return pcpp::MacAddress(body->macAddress); |
There was a problem hiding this comment.
nit: Do we need the pcpp namespace qualification here?
| pcpp::IPv4Address getIpAddress() const | ||
| { | ||
| auto body = this->getGvcpDiscoveryBody(); | ||
| return pcpp::IPv4Address(body->ipAddress); |
There was a problem hiding this comment.
nit, ditto: namespace qualification
| pcpp::IPv4Address getSubnetMask() const | ||
| { | ||
| auto body = this->getGvcpDiscoveryBody(); | ||
| return pcpp::IPv4Address(body->subnetMask); |
There was a problem hiding this comment.
nit, ditto: namespace qualification
| pcpp::IPv4Address getGatewayIpAddress() const | ||
| { | ||
| auto body = this->getGvcpDiscoveryBody(); | ||
| return pcpp::IPv4Address(body->defaultGateway); |
There was a problem hiding this comment.
nit, ditto: namespace qualification
| pcpp::MacAddress getMacAddress() const | ||
| { | ||
| auto body = this->getGvcpForceIpBody(); | ||
| return pcpp::MacAddress(reinterpret_cast<const uint8_t*>(body->macAddress)); |
There was a problem hiding this comment.
nit, ditto: namespace qualification
| pcpp::IPv4Address getIpAddress() const | ||
| { | ||
| auto body = this->getGvcpForceIpBody(); | ||
| return pcpp::IPv4Address(body->ipAddress); |
There was a problem hiding this comment.
nit, ditto: namespace qualification
| pcpp::IPv4Address getSubnetMask() const | ||
| { | ||
| auto body = this->getGvcpForceIpBody(); | ||
| return pcpp::IPv4Address(body->subnetMask); |
There was a problem hiding this comment.
nit, ditto: namespace qualification
| pcpp::IPv4Address getGatewayIpAddress() const | ||
| { | ||
| auto body = this->getGvcpForceIpBody(); | ||
| return pcpp::IPv4Address(body->gateway); |
There was a problem hiding this comment.
nit, ditto: namespace qualification
Per #1321
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.