Skip to content
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

OpParams as a standalone process #105

Open
jamcar23 opened this issue Jul 22, 2021 · 1 comment
Open

OpParams as a standalone process #105

jamcar23 opened this issue Jul 22, 2021 · 1 comment
Labels
concept Future feature ideas / concepts enhancement New feature or request

Comments

@jamcar23
Copy link
Owner

jamcar23 commented Jul 22, 2021

The purpose of this issue is to outline the ideas, goals, and challenges of rewriting opparams to be its own dedicated process.

Goals

  • Allow opparams to evaluate the params internally
    • I shouldn't need to check if the param is enabled everywhere I want to use it, opparams should handle this
    • Ideally opparams doesn't just tell me if it's enabled but can actually return the correct value when it's not
      • e.g. I should be able to replace CP.steerActuatorDelay with op_params.eval(STEER_ACTUATOR_DELAY) (generalized to any prop from any message / state) in any location with no other code changes (import statement for OpParams will still be needed of course)
  • Live params in C/C++
    • Selfdrive only, Panda will have to continue to use static / compile time params
  • Safer param file access
    • Only 1 process (opparamsd) should read / write the param file(s)
    • Each param should be stored in separate files in order to minimize risk & scope of file corruption

Challenges

  • Cereal will have to be used for inter-process communication
    • Will all possible params in opparams be serializable as capt 'n' proto messages?
      • How do we handle params which can't? Just not allow them via opEdit?
      • Will this require different messages for different supported datatypes? If so, how should we handle the different message types in C/C++? (Assuming this is a situation where two different params could be used, i.e. number v. list of numbers.)
    • OpEdit will need to use pub/sub masters to talk to OpParams
      • Will enough of the Param message be serializable so OpEdit can still function (with at least the current level of functionality)?
        • If not:
          • should we use strings and runtime code generation to allow OpEdit to have the full param class?
          • should we have opparamsd handle validation and communicate with OpEdit in a request/response pattern?
  • How will opparams know which params are needed to evaluate the param?
    • e.g. to evaluate STEER_ACUATOR_DELAY it needs to look at ENABLE_LAT_PARAMS, ENABLE_STEER_ACTUATOR_DELAY, ENABLE_ACTUATOR_DELAY_BPS_MULTI, STEER_ACTUATOR_DELAY_BP_MULTI, STEER_ACTUATOR_DELAY_V_MULTI, STEER_DELAY_MULTI_BP_SOURCE, ENABLE_ACTUATOR_DELAY_BPS, STEER_ACTUATOR_DELAY_BP, STEER_ACTUATOR_DELAY_V, and STEER_ACTUATOR_DELAY; how will opparams know all this?
    • Perhaps Params could use a lambda / function to handle evaluation on a per param basis?
    • Maybe there should be specific param class for the different types of param groups?
      • e.g. a MultiBpParam which handles evaluating the params
        • if we do this the param group class should be able to declare all need params to make the feature work (e.g. breakpoints will always need BP and V params.)
  • Similarly, how will it know which OP message / state to use when evaluating an opparam?
    • e.g. how does it know to use CP.steerActuatorDelay if ENABLE_STEER_ACTUATOR_DELAY is false?
    • Perhaps there is a fallback prop for the param group with a string in the format of [message].[field] which then gets converted to the submaster equivalent string and then uses the same runtime code generation as opEdit to turn it into python?
      • e.g. carParams.steerActuatorDelay string becomes sm["carParams"].steerActuatorDelay string where sm is an existing SubMaster instance inside of opparamsd once the code is generated
  • Automated tests will be harder as all processes would now depend on opparamsd (and whatever it depends on)

Implementation Ideas

  • The existing opParams class & functionality will be need to be split across at least 2 classes: OpParamsd and OpParamsListener (final names tbd)
    • OpParamsd
      • contains all functionality related to reading / writing param files, evaluating params, and anything else related to the management / control of the params directly
      • should publish 3 messages: OpParamsEvaluation, OpParamsRaw, OpEditResponse (final names tbd)
        • Evaluation should contain the pre-evaluated params
          • used by processes (controlsd, planner, etc.) which only need to know the final value that should be used
        • Raw should contain the complete Param class
          • used by processes (OpEdit, UI (eventually)) which need to know / display all param information
        • OpEditResponse will be the response from opparamsd when OpEdit request some param change
          • not 100% sure what will go here or what will be needed
          • if OpEdit has its own complete copy of the Param it may be able validate the new values inside its own process in which case a response may not be needed
      • should subscribe to all OP messages + any new messages needed for OpEdit (e.g. OpEditParamChangeRequest)
    • OpParamsListener
      • this will be the version of OpParams that is used outside of opparamsd
      • publishes 0 messages; subscribes to OpParamsEvaluation (perhaps OpParamsRaw optionally too)
      • should contain all code needed for single line replacement of any OP prop in any location (goal 1.2)
  • OpEdit will need to be reworked to keep existing functionality while not using OpParams directly
    • Subscribes to OpParamsRaw and OpEditResponse (if needed)
    • Publishes OpEditParamChangeRequest
      • used by opparamsd to listen for when the user is changing param values
    • In addition to things like check if the entered value has the correct expected type, OpEdit will need to validate that the data type and structure can be serialized as a capt 'n' proto message. These can be done two ways:
      • 1st. OpEdit can send a change request to opparamsd which then validates the change request and then sends a response back to OpEdit
        • this would absolutely require the request & response messages as OpEdit has to alert the user in the event of errors
        • this would probably be the more performant option (at runtime)
      • 2nd. OpEdit could use the data inside OpParamRaw to generate its own copy of the Param python classes and then validate new values directly
        • this would essentially use the same Param code as opparamsd without any of the read / write functionality (or the ability to update the params used by other processes)
          • since OpEdit would be creating a second copy of the params classes (as well as possibly using runtime code generation) option 1 is probably more performant
        • a response message may not be needed because OpEdit already knows the data is valid and will be accepted by opparamsd
@jamcar23 jamcar23 added enhancement New feature or request concept Future feature ideas / concepts labels Jul 22, 2021
@jamcar23
Copy link
Owner Author

jamcar23 commented Aug 6, 2021

After speaking with some other members of the community I've decided that captn proto serialization is probably the easiest problem as there are multiple solutions. The easiest being to convert the code for the value / param into a string, serialize it into a hashmap, and then use runtime code generation to convert it back into python on the other side. There are other options like having opparamsd write the evaluated value into param text file (kinda like how Params works already) but either way, it's relatively easy to solve.

The bigger challenges are going to be 2 & 3 above, specifically: how will params be grouped together and how will opparams know how to evaluate them?

For challenge 2 I think some combination of 2.2 & 2.3 will work best, starting with 2.3 (param group class). Specifically:

  • Perhaps Params could use a lambda / function to handle evaluation on a per param basis?
  • Maybe there should be specific param class for the different types of param groups?
    • e.g. a MultiBpParam which handles evaluating the params
      • if we do this the param group class should be able to declare all need params to make the feature work (e.g. breakpoints will always need BP and V params.)

For challenge 3, 3.2 is still the best / only option I've thought of. I'm not sure if this will be the best approach, it would be a good idea to revisit this after solving challenge 2.

  • Perhaps there is a fallback prop for the param group with a string in the format of [message].[field] which then gets converted to the submaster equivalent string and then uses the same runtime code generation as opEdit to turn it into python?
    • e.g. carParams.steerActuatorDelay string becomes sm["carParams"].steerActuatorDelay string where sm is an existing SubMaster instance inside of opparamsd once the code is generated

Roadmap challenge 2:

  • Extend params class with breakpoint class
  • Extend breakpoint class with multi breakpoint class
  • Refactor existing param declarations to work with new classes
  • Make needed opparams changes so that these new params are backwards compatible for all other code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concept Future feature ideas / concepts enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant