servod: clearing up design decisions on guidelines on controls and state |
||
Issue descriptionThis bug is intended mainly to be a discussion to see if we want to formalize a certain behavior for controls in servod. Background here is that there are two types of controls in servod: - ones that take arguments, and ones that don't - or set/get controls. I think initially there wasn't a big need for a distinction as servo(d) was meant to essentially deal very closely with the hardware, toggling GPIOs etc. (please correct me if that understanding is faulty). In that world, it's equivalent to say get == no arguments == no state change. As servod has evolved the following things have started to creep in: - controls that change state and are either set/get - controls that really are a sequence of controls, so they set/get multiple things. And do more complex tasks essentially a short-hand for common pathways (e.g. recently enforcing that downloading an image to a usb mux means to point the mux to the servo & turn power on for it). Just some ideas here: - There's value in a clean set/get distinction to know when you're doing something vs when you're just querying for information - The current mechanism of having a control take at most one argument prompts generally better designed controls, as they don't get bloated with unnecessary arguments that should be a default - Maybe set can be triggered with an optional argument, instead of requiring one - For dut-control this would imply that dut-control control_name: would be a set without an argument. Maybe that's an awkward way to do that however. - there might be value in reviving Todd's old TODOs to implement a <sequence> ability inside of config files to remove some of the controls that essentially just a sequence of things - lastly, slightly unrelated, but a lot of our controls in ccd/micro/v4 world have become ec console controls that issue a command, check against a regex, and report something etc. There is definitely value in creating a generic, tested way to do this kind of common procedure, and have the controls all use the same at least base driver, for this kind of thing. Please share your thoughts on the above, and feel free to CC whoever might be interested/helpful
,
Oct 19
On the 3rd point, before I forget the thought this morning: For example https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec.py#80 https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec.py#93 https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec.py#106 https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec.py#123 https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec.py#264 https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec.py#324 https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/master/servo/drv/ec3po_servo_v4.py#137 (and many more, just picking the first few I remembered). While this is fine, they don't behave uniformly e.g. some limit the channel, some don't, some use the regex with built-in groups, others use a list of regex. I think there is value in having a "base ec3po"(not married to the name). That would allow us to pull all this logic into the xml files i.e. have the regex, the match groups, all that in the xml file. That might not be able to get rid of all ec-related controls, but it would cut down the size of the boilerplate code a lot, and reduce the number of issues encountered due to some ec controls behaving differently than others.
,
Oct 24
What do you think moving dut-control to explicit get vs set commands as part of this? E.g.: dut-control get dut_i2c_mux dut-control set dut_i2c_mux ec_prog (Yes I deliberately got rid of the colon separator there.) I think supporting console commands purely from XML config should be tracked in a separate bug from splitting the get vs set and arg vs no_args distinctions. (Which bug this one becomes, up to you!)
,
Oct 25
This might be unrelated, but: autotest has support for talking to servod, so we need to make sure dut-control parsing changes don't affect autotest libraries.
,
Oct 25
For #3, I would very much expect a period of overlapping support for both old and new syntax, while updating all automation (e.g. autotest) and documentation. Regardless of exact CLI syntax, I like the idea if separating the get vs set concept from arg(s) vs no-args. Implementing enable_ite_dfu as a "get" method felt strange and disingenuous, but the alternative of making it take an unused arg felt more wrong.
,
Oct 25
#2, based on those examples it does indeed look like a refactor there is in order to cleanup the various differences. #3, #5, not sure I see reward in making the change over the amount of headache it could potentially cause. Would something like implementing the ITE DFU mode as dut-control ite_dfu ite_dfu:disable dut-control ite_dfu:enable Be another alternative?
,
Oct 25
There is no disable ITE DFU functionality in servod. IIRC it would be possible to implement, but I know of no reason to spend on it, so I haven't. I could change it to take enable/disable or on/off values and just give an error when attempting to actually use disable/off. Thoughts?
,
Nov 22
|
||
►
Sign in to add a comment |
||
Comment 1 by tbroch@chromium.org
, Oct 18