E1.33 cherry pick third time lucky #1947
Conversation
…nto e1.33-cherry-pick
(cherry picked from commit f3b7511)
…messages (cherry picked from commit 1955638)
(cherry picked from commit 32ecc52)
(cherry picked from commit 8db5131)
(cherry picked from commit c2126d1)
(cherry picked from commit 2a43710)
(cherry picked from commit 19eca6f)
(cherry picked from commit f49c1f8)
(cherry picked from commit 5b36363)
(cherry picked from commit eb609f3)
(cherry picked from commit e99ec91)
(cherry picked from commit d40a36a)
…mised (cherry picked from commit ad9ac0d)
Tests still required for these! (cherry picked from commit 9fab749)
(cherry picked from commit 6d4d923)
(cherry picked from commit 81b8e38)
(cherry picked from commit b23bc6b)
|
|
||
| // The max size of an E1.33 Status string. | ||
| enum { | ||
| MAX_E133_STATUS_STRING_SIZE = 64 |
There was a problem hiding this comment.
Mh, you use single-element enums here and static const uint8_t in include/ola/rdm/RDMEnums.h (see MAX_RDM_DOMAIN_NAME_LENGTH). Is this on purpose or an inconsistency? Same for E133_VERSION below.
There was a problem hiding this comment.
Ask @nomis52 from 11 years ago! 😆
https://github.com/OpenLightingProject/ola/blame/f0b01b359b4bfc3e24eb0a7a2aa96028638128ef/include/ola/e133/E133Enums.h#L95
I just copied it for the version, but I guess we could theoretically support a few versions at the same time so maybe they should be an enum at least?
There was a problem hiding this comment.
Sure, if multiple values make sense, keep it as an enum. How about this MAX_STRING_SIZE?
There was a problem hiding this comment.
I'm not entirely sure they do necessarily. I've kicked it down the road a bit and added a todo in #1841 anyway.
|
I've pushed. Do you want to re-review or are you happy if I just merge @kripton ? |
You can merge 👍 . If I approve on the first review, it's just minor stuff that I trust people to fix (or at least triage) before they merge. If it something I want to check again, I finish the review with "Comment only, no approval" :) |
Cool, I think I'll park a chunk of the more complex ones or those that require a bit more thought/clean-up into the main issue to try and avoid conflicts and tidying up multiple times as different bits are merged. I think we're only around 1/3 to 1/4 of the way through so far unfortunately.
Or maybe you could use one of the specific options GitHub offers, then it's not actually possible to merge until you get an approval? :)
|
|
Force merging as the CI error is already covered in #1948 . |
But maybe not for you, apologies I probably pulled a bit too much into this sorry.
Let me know if it's too much and I'll rethink somehow...
More of #1841