Skip to content
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

Merged
merged 6 commits into from
Feb 28, 2024

Conversation

peternewman
Copy link
Member

Plus the additional E1.33 and E1.37-7 PIDs that enables

@peternewman peternewman added this to the 0.11.0 milestone Dec 3, 2023
Copy link
Member

@kripton kripton left a 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!

common/messaging/SchemaPrinterTest.cpp Show resolved Hide resolved
@@ -40,6 +40,7 @@ class MessagePrinter: public MessageVisitor {

virtual void Visit(const BoolMessageField*) {}
virtual void Visit(const IPV4MessageField*) {}
virtual void Visit(const IPV6MessageField*) {}
Copy link
Member

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

Copy link
Member Author

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:

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.

Copy link
Member

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.

@peternewman peternewman merged commit 522c5a9 into OpenLightingProject:master Feb 28, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants