-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for the IPv6 data type to the RDM messaging #1926
Add support for the IPv6 data type to the RDM messaging #1926
Conversation
…ional E1.33 and E1.37-7 PIDs that enables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looking good. The only thing is IPV4Message and IPV6Message vs. IPv4Message and IPv6Message for better readability. But perfectly fine otherwise!
@@ -40,6 +40,7 @@ class MessagePrinter: public MessageVisitor { | |||
|
|||
virtual void Visit(const BoolMessageField*) {} | |||
virtual void Visit(const IPV4MessageField*) {} | |||
virtual void Visit(const IPV6MessageField*) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to have "IPV6MessageField" instead of "IPv6MessageField"? I mean, sure, IPV4 has been here before but IPv4 and IPv6 are easier to read then IPV4 and IPV6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just legacy/compatibility for now really, we've generally gone for either CamelCase or being consistently upper-cased, and IPVx tracks through a lot of other code too, e.g. IPV4SocketAddress
I find DmxBuffer more jarring personally!
Also we should probably prioritise fixing this, which renders out user-facing code before worrying too much about our internal names:
ola/common/utils/StringUtils.cpp
Lines 437 to 453 in 6534491
void CustomCapitalizeLabel(string *s) { | |
// Remember to update the Doxygen in include/ola/StringUtils.h too | |
static const char* const transforms[] = { | |
"dhcp", | |
"dmx", | |
"dns", | |
"ip", | |
"ipv4", // Should really be IPv4 probably, but better than nothing | |
"ipv6", // Should really be IPv6 probably, but better than nothing | |
"led", | |
"mdmx", // City Theatrical, should really be mDMX, but better than nothing | |
"pdl", | |
"pid", | |
"rdm", | |
"uid", | |
NULL | |
}; |
Although at least C++ has it, the Python doesn't have anything from memory. Obviously ideally there would be one place, that could ideally handle the more complicated cases, for this to be managed. probably via a shell script that generates an include file or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, IPV4SocketAddress is fine for me (as is DmxBuffer :D). It was just for better readability since IPv4 and IPv6 are much more commonly used.
522c5a9
into
OpenLightingProject:master
Plus the additional E1.33 and E1.37-7 PIDs that enables