Skip to content

Refactor Embedded Data On Robot Diagnostics CLI#3628

Closed
a-png129 wants to merge 1 commit intoUBC-Thunderbots:masterfrom
a-png129:alice/refactor_primitive_factory
Closed

Refactor Embedded Data On Robot Diagnostics CLI#3628
a-png129 wants to merge 1 commit intoUBC-Thunderbots:masterfrom
a-png129:alice/refactor_primitive_factory

Conversation

@a-png129
Copy link
Copy Markdown
Contributor

@a-png129 a-png129 commented Feb 28, 2026

Concerns I have with this design

  • I was having trouble with the factory pattern because the different get_primitives take different arguments. e.g. get_move_primitive(angle, speed), get_chip_primitive(auto, distance), so it's not a perfect black box. There's a lot of responsibility on the caller to pass in the right arguments, and the dynamic string dispatch makes it feel very fragile -- weakened type clarity, increased risk of runtime failure
  • Factory has an inconsistent return type (Primitive for most, PowerControl for get_zero_power_control_primitive, MotorControl for get_zero_motor_control_primitive)
  • The goal of the refactor was to simplify and make it make more sense... but I feel like this design doesn't really accomplish that. It centralizes construction, but introduces complexity in call semantics

Instead of having one central creation method (get_primitive), I'm thinking of keeping the methods separate and explicit:
factory.get_move(...)
factory.get_rotate(...)
Essentially, just moving those methods from embedded_data to a different file... giving up on centralized construction

===============================================================================

Description

Testing Done

Resolved Issues

Length Justification and Key Files to Review

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

  • Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
  • Remove all commented out code
  • Remove extra print statements: for example, those just used for testing
  • Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

@Andrewyx
Copy link
Copy Markdown
Contributor

Andrewyx commented Apr 7, 2026

@a-png129 bumping this PR, what is the current status of this issue? Can it be closed if it is not currently being maintained?

@a-png129
Copy link
Copy Markdown
Contributor Author

@Andrewyx sorry I missed this! I basically got blocked by 1) not knowing what direction to go with the design... and 2) the error I encountered when I tried flashing the robot to test it: ImportError: /home/robot/thunderbots_binaries/robot_diagnostics_cli.runfiles/_main/software/py_constants.so: undefined symbol: PyFrame_GetCode

I'm good with closing this for now, though, as it doesn't seem like an important task that needs attention atm

@a-png129 a-png129 closed this Apr 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants