-
Notifications
You must be signed in to change notification settings - Fork 11
Initial E1.37-5 PID definitions #315
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
Conversation
| # LIST_TAGS | ||
| {'get_request': {'items': []}, | ||
| 'get_response': {'items': [{'name': 'tags', | ||
| 'type': 'string'}]}, |
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.
Wouldn't mind a second opinion on this @kripton .
It's a string, but it's also a null delimited list of tags. E.g. foo\0bar\0baz\0 . Always with the trailing null according to the spec.
The rest of the E1.20 text handling stuff says when you have a string and you see a null, you truncate on it, so our stuff all does this, so getting the above list just gives foo. 😞
Thoughts, do we add some special flag to a strings to say it may contain nulls and don't touch them, or make this a group of 1 character strings or bytes or something quirky?
I guess maybe we should see how E1.37-5's JSON stuff ends up handling it; I don't think they've a plan yet either...
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.
I would say it depends on which characters are allowed for a tag. If commas or semicolons are not allowed, we could use those as separators, only sending the spec-conform, \0-delimited string on the wire and transforming it back right when we received it.
If JSON is expected, usually dots or quotation marks are not allowed, even though they could be escaped
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.
I think the spec says any character apart from \0 is allowed.
This is sort of what this should do. Normally this takes the byte stream and converts it into the "raw" types (e.g. uint/string/UID etc), then there's another level above which can do some additional pretty printing (e.g. rendering the clock fields as a timestamp, rather than individual fields, in https://github.com/OpenLightingProject/ola/blob/master/include/ola/rdm/RDMMessagePrinters.h ).
So I guess ideally we'd be passing back an array of strings (split from the nulls), but then we get lots of edge cases of one "item" essentially needs two sets of properties, it's combined max length, and then the max length of each of the to be produced array elements. The delimiter etc.
Well TBH ideally they'd have picked something like the record separator (as I suggested) to avoid this issue).
I guess it's a case of where we cause lots of chaos, in the base string object processing, which then needs to produce either a simple string or an array of strings (and all the knock ons with sizing of groups etc). Or downstream in the RDM tests and printer (if we just return an array of single byte values and post-process).
The E1.33 scope stuff feels a bit easier, as it's just a case of being able to null pad a string, so it's always 63 octets long.
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.
Actually maybe following E1.37-5's JSON does sort of hold the answer. They've put it as bytes, which would be equivalent to our array of uint8 or chars, as I was proposing before, and which we can unpick to what we want in the higher level...
No description provided.