E1.33 cherry pick the second #1945
Conversation
(cherry picked from commit b176ba1)
(cherry picked from commit 1a027ac)
(cherry picked from commit 9845e37)
(cherry picked from commit 7c44c8b)
(cherry picked from commit d01494b)
(cherry picked from commit eb97fb4)
(cherry picked from commit 356decf)
…dard (cherry picked from commit 35e4ed1)
(cherry picked from commit e9dd374)
…ransport (cherry picked from commit 33ea03f)
| /** | ||
| * @brief The port used for E1.33 LLRP communication. | ||
| */ | ||
| const uint16_t LLRP_PORT = 5569; |
There was a problem hiding this comment.
it's the same number as above. Do we need both? E133_PORT and LLRP_PORT?
There was a problem hiding this comment.
I think E133_PORT becomes redundant looking at where it's used, and they essentially reused the port they'd chosen/had allocated.
I'm inclined to leave them separate for now and I've added a task to the primary PR to remove all the redundant code from the pre-standards version.
| bool IntToConnectStatusCode(uint16_t input, | ||
| E133ConnectStatusCode *connect_status_code) { | ||
| switch (input) { | ||
| case ola::e133::CONNECT_OK: |
There was a problem hiding this comment.
how about:
case ola::e133::CONNECT_OK:
case ola::e133::CONNECT_SCOPE_MISMATCH:
.......
*connect_status_code = input;
return true;
?
There was a problem hiding this comment.
That's a very elegant solution.
Unfortunately that code doesn't solely exist in E1.33 land. Do you fancy a clean-up of this (once merged) and the other similar code (e.g. there's some in https://github.com/OpenLightingProject/ola/blob/master/common/rdm/RDMHelper.cpp )?
We'd also need to add OLA_FALLTHROUGH to cover us from other compiler warnings, but looks neat.
There was a problem hiding this comment.
I can have a look, yes. Ping me once you merged it!
There was a problem hiding this comment.
It's in now @kripton but I assume you got an automatic notification anyway...
Thanks.
Yeah sorry it was a bit lumpy! Do you want to check my replies and double-check nothing else snuck through whilst you were looking? |
No problem. It should be fine, github makes it quite easy to track what changed after I started reviewing so go ahead merging it 👍 |
The next sub-part of #1841