Refloat: LCM now supports "lights_off_when_lifted" #6
Conversation
surfdado
force-pushed
the
main
branch
2 times, most recently
from
1947ab6 to
de45c79
Compare
refloat/refloat/lcm.c
Outdated
| @@ -31,6 +31,7 @@ void lcm_init(LcmData *lcm, CfgHwLeds *hw_cfg) { | |||
| lcm->status_brightness = 0; | |||
| lcm->name[0] = '\0'; | |||
| lcm->payload_size = 0; | |||
| lcm->lights_off_when_lifted = 1; | |||
There was a problem hiding this comment.
Please assing true, as it's a bool value, it's cleaner that way.
| buffer[ind++] = fminf(100, fabsf(motor->duty_cycle * 100)); | ||
| } else { | ||
| // pitch is a value between -180 and +180, so abs(pitch) fits into uint8 | ||
| buffer[ind++] = lcm->lights_off_when_lifted ? fabsf(pitch) : 0; |
There was a problem hiding this comment.
This is not a very good way to do it, as you've just combined two values into one (the off_when_lifted flag and pitch). If someone would e.g. want to display status when lifted (the way Refloat does it), this would be useless to them and would need changing.
There was a problem hiding this comment.
true - that would be impossible. On the floatwheel the front lights are just on/off anyway and not addressable. This API is truly meant for just the LCM. In the mid-term we need one that targets any external LED controller, assuming that it has the same kind of feature set as what's supported internally
| // pitch is a value between -180 and +180, so abs(pitch) fits into uint8 | ||
| buffer[ind++] = lcm->lights_off_when_lifted ? fabsf(pitch) : 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm very queasy about this addition to the protocol. The command interfaces need to be maintained in a consistent way and their binary layout designed with some clean concept. I wanted to draft some proposal for the protocol(s) but haven't gotten to it yet.
I think the only way I'm willing to accept this now is if we agree these interfaces are temporary and will be replaced at some point... But I'd maybe prefer to spend some more time on the change and get it into the next version.
The particular issue here is there's no clean definition of which values are sent when, you've added a condition on this one to be different between RUNNING and other states, but below this there are some RUNNING values still out of the condition.
I assume there are also more consumers of this interface besides Floatwheel LCM, so the protocol should be somehow agreed on by all parties, not just organically grow with whichever value someone wants to add and just finds a vacant spot for it...
There was a problem hiding this comment.
I am totally fine with this being temporary. This API is currently very narrowly targeting the floatwheel LCM and not very well suited for generic use. Also, you've seen Mitch's request for an API to query LED config data so we do need to come up with something that is more generic.
Signed-off-by: Dado Mista <dadomista@gmail.com>
No description provided.