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

Enlargable Config Packet #163

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

Conversation

Apehaenger
Copy link
Collaborator

@Apehaenger Apehaenger commented Oct 14, 2024

In relation to ClemensElflein/OpenMower#110

Roadmap:

  • Decide for config protocol and struct
  • LowLevel (OpenMower Pico-FW) please see Enlargeable Config Packet OpenMower#110
  • HighLevel (open_mower_ros)
    • Implement packet handling
    • Implement config value handling
      • Charge & Battery
      • Hall sensor inputs
      • Emergency
      • Misc
  • Test with old LL-FW

🔨 = Currently working on

@Apehaenger
Copy link
Collaborator Author

Apehaenger commented Oct 18, 2024

Intermediate commit nr.2 is only for illustration purposes

What's in

  • Flexible length config packet is straight forward and works
  • Config Packet handling HL/LL, LL/mower_logic is illustrated by two LL specific x_charge_* config values

What has changed

In past the (old) config packet request/response phase got trigger by LL whenever ROS became live (detected via heartbeat packet). This got reverted now because it will save at least one packet.
Read on how it's now...

General config packet handling

Whenever HL detects missing LL-status packets for > 1 sec. it assumes that the mower just booted or LL rebooted i.e. due to flashing a new FW, and send a config packet (and response request) with HL's configured LL values, which are normally not any!

Yes, (in my opinion) HL does not need to know any specific LL values in >90?%, because it is HL.
LL knows about it's default C500 configuration values.
Except in the rare cases where someone is using a custom mower, battery, hall-config or whatever.
In those case, the individual LL values got placed in mower_config (not yet implemented. Only dynamic reconfigure parameters got added for illustration and tests) and get send within the initial config request packet.

LL will receive this HL-config packet, do a sanity check where it has hardware limits like (charge cutoff voltage/current), and send the used/adjusted valued back to HL as "response".

When HL receives LL's response, dependent on what/if something changed in comparison to what requested before, or did not know before (because HL does not know much about LL), it will send a dynamic reconfigure and inform mower_logic about the relevant/changed values, which are used i.e. for battery-low calculations (go home), sensor view, ...

Similar will happen if one is changing values via dynamic reconfigure (mower_logic). Change get compared with what is in, if changed will inform LL with a new config (including the response request) and will align once response get received.

Did a couple of tests with the two implemented vales. Works pretty good. A full packet round, even if with a to large value, take < 1s.

What's bad

I don't like the static member checks in manageDynReconfConfigValues() and manageLowLevelConfigValues().
Would like to iterate over the config struct members, but for this it looks like boost is required. See FIXME comment within these function. Any tips how it could be made more dynamic are welcome! 😇

@Apehaenger Apehaenger marked this pull request as ready for review October 26, 2024 19:25
@Apehaenger
Copy link
Collaborator Author

Same here. In my opinion, all is done.
Did a couple of tests with old-LL/new-HL, new-LL/old-HL, new-LL/new-HL on C500 as well as SA600? mower.
Haven't found any issue.

Would love to see a generic review before asking for tester @ discord.

Copy link
Owner

@ClemensElflein ClemensElflein left a comment

Choose a reason for hiding this comment

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

LGTM

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