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

Remove CyclerOutput from Communication Client and changes the communication protocol to only use the String type for paths #709

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

oleflb
Copy link
Contributor

@oleflb oleflb commented Jan 31, 2024

Introduced Changes

Removes the CyclerOutput from the communication client. This means we can add features on the server side without modifying the client.

Fixes #

ToDo / Known Issues

If this is a WIP describe which problems are to be fixed.

Ideas for Next Iterations (Not This PR)

If there are some improvements that could be done in a next iteration, describe them here.

How to Test

  • Check that all twix panels and overlays still work ;)

@oleflb oleflb enabled auto-merge January 31, 2024 20:10
@oleflb oleflb changed the title Remove CyclerOutput from Communication Client Remove CyclerOutput from Communication Client and changes the communication protocol to only use the String type for paths Jan 31, 2024
Copy link
Member

@h3ndrk h3ndrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in chat, this PR breaks some architecture principles in type-safety of the communication protocol. The separation of output variant and path was intentional because we would otherwise mix concepts that don't belong together. Free string fields in protocols are usually a design smell which this PR violates. Please don't ignore Chesterton's fence.

PR #710 is now abusing this violation. As mentioned in chat, execution times have nothing to do with the data which cyclers produce and exchange. Timing information is orthogonal and should be exposed via a separate communication service. This allows further extension (e.g. profiling and more timing measurements) be made easily which is not possible with your proposed solution. Otherwise we repeat the architectural mistakes we currently have in the behavior simulator.

@oleflb
Copy link
Contributor Author

oleflb commented Feb 1, 2024

I am open to implement this differently. We should integrate @knoellle and @schmidma in this decision making process as they are far more experienced than myself but don't like the current implementation (on main) either.

@h3ndrk
Copy link
Member

h3ndrk commented Feb 2, 2024

That's why I started the discussion publicly here 👍

@oleflb
Copy link
Contributor Author

oleflb commented Feb 14, 2024

@okiwi6, @h3ndrk and @knoellle discussed this today.
@h3ndrk argues the cycler and output information is different from the path information and should be treated as such.
@okiwi6 and @knoellle argue that the representation should be transparent to users, so paths like control.main_outputs.sensor_data still make sense despite its components having different internal meanings.
However this is implemented, the cycler has to be transmitted via string (or other identifier) which is only available at runtime since there should not be a dependency of the client on the code on the robot. The output field could be implemented as enum or string. The remaining_path also has to remain a string since it's also determined at runtime.
The approach in this PR, having everything in one String requires more parsing in the server, which @h3ndrk dislikes.
Also needs further discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants