Enlargeable Config Packet

Apehaenger commented 1 month ago

Roadmap:

[x] Decide for flexible config packet and struct
[x] LowLevel (OpenMower Pico-FW)
- [x] Implement packet handling
- [x] config save in flash via LittleFS (rovo89) :heart_eyes:
- [x] Implement config value handling
- [x] Charge & Battery
- [x] Hall sensor inputs
- [x] Emergency handling
- [x] Ignore_charging_current
- [x] Misc (ignore charging current, ...)
[x] HighLevel (open_mower_ros) please see https://github.com/ClemensElflein/open_mower_ros/pull/163
[x] Test with old HL-FW _{:hammer: = Currently working on}

Test:

Each side (LowLevel-Pico / HighLevel-Raspi) should be upgrade-able without any compatibility issues. But for getting the new configuration possibilities, both sides need to be updated.

LowLevel (Pico)

Firmware can be found within Actions. Select the newest Enlargeable Config Packet workflow. Even if it has a red cross in front, it should contain the open-mower-pico-firmware package within the "Atrifacts" section (scroll down).

HighLevel (Raspi)

Docker image can be pulled via sudo podman pull ghcr.io/clemenselflein/open_mower_ros:pr-163 (don't forget to change your /boot/openmower/openmower_version.txt to 'pr-163')

Configuration workflow

Once HighLevel is up and detect a running LowLevel, it will send a "configuration wish-list" to LowLevel like:

Oct 30 17:37:08 Dolly openmower_launch[17587]: [ INFO] [1730306228.494246883]: Send ll_high_level_config packet 0x11
Oct 30 17:37:08 Dolly openmower_launch[17587]:          options{dfp_is_5v=0, background_sounds=0, ignore_charging_current=0},
Oct 30 17:37:08 Dolly openmower_launch[17587]:          v_charge_cutoff=-1.000000, i_charge_cutoff=-1.000000,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          v_battery_cutoff=-1.000000, v_battery_empty=24.000000, v_battery_full=28.000000,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          lift_period=65535, tilt_period=65535,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          shutdown_esc_max_pitch=255,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          language="en", volume=255
Oct 30 17:37:08 Dolly openmower_launch[17587]:          hall_configs="U, U, U, U, U, U, U, U, U, U"

LowLevel receive this "config packet", aligns it with his defaults values as well as his known hardware limits and answer with a config-response-packet containing the applied, adapted or default values, like:

Oct 30 17:37:08 Dolly openmower_launch[17587]: [ INFO] [1730306228.505676744]: Received ll_high_level_config packet 0x12
Oct 30 17:37:08 Dolly openmower_launch[17587]:          options{dfp_is_5v=0, background_sounds=0, ignore_charging_current=0},
Oct 30 17:37:08 Dolly openmower_launch[17587]:          v_charge_cutoff=30.000000, i_charge_cutoff=1.500000,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          v_battery_cutoff=29.000000, v_battery_empty=24.000000, v_battery_full=28.000000,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          lift_period=100, tilt_period=2500,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          shutdown_esc_max_pitch=0,
Oct 30 17:37:08 Dolly openmower_launch[17587]:          language="en", volume=80
Oct 30 17:37:08 Dolly openmower_launch[17587]:          hall_configs="!L, !L, !S, !S, I, I, I, I, I, I"

Configuration

Please check the sample config file regarding the possible new parameters, like Battery & Charge, Hall/Emergency or Ignore Charging Current.

rovo89 commented 1 month ago

Seems like you're not using anything from debug.h now, except the initialization. 🤔

rovo89 commented 1 month ago

IGNORE_CHARGING_CURRENT could also become a setting I guess. Maybe we can have only a single build in the future?

rovo89 commented 1 month ago

Seems like you're not using anything from debug.h now, except the initialization. 🤔

Ah, in main.cpp, there are many places with #ifdef USB_DEBUG DEBUG_SERIAL.println(...). Did you mean to replace those? But that would be unrelated to this PR. So in the interest of keeping this PR focused on the extended config, I'd suggest to undo the DEBUG_BEGIN change and remove debug.h. If you feel that it simplifies stuff, it can be introduced in another PR.

Apehaenger commented 1 month ago

Seems like you're not using anything from debug.h now, except the initialization. 🤔

No, not anymore, since you deleted nv_config :-)

IGNORE_CHARGING_CURRENT could also become a setting I guess. Maybe we can have only a single build in the future?

I already had that discussion in a similar way a couple of weeks ago. I liked to move it to checkShouldCharge() and don't check for the current, but got stopped with something like "better no values" than "wrong values" (even if the relevant ppl know their hardware failure)... But wait... or do you mean those could configure/send i_charge_cutoff = 0 and we disable the cutoff logic based on the current? No, I wouldn't do that. All ppl. could disable the charge logic by this. Yes, all people can also flash *_IGNORE_CHARGE_CURRENT, that's true :thinking: . Please ask @ClemensElflein for this, I wouldn't do that, simply because the name kindly sound dangerous and the normal healthy use will not install that version.

Seems like you're not using anything from debug.h now, except the initialization. 🤔

Ah, in main.cpp, there are many places with #ifdef USB_DEBUG DEBUG_SERIAL.println(...). Did you mean to replace those? But that would be unrelated to this PR. So in the interest of keeping this PR focused on the extended config, I'd suggest to undo the DEBUG_BEGIN change and remove debug.h. If you feel that it simplifies stuff, it can be introduced in another PR.

Will do with the next commit ;-)

rovo89 commented 1 month ago

But wait... or do you mean those could configure/send i_charge_cutoff = 0 and we disable the cutoff logic based on the current?

I hadn't checked in much detail - just thought that we could try and reduce the number of build variants. 😉

IIUC, IGNORE_CHARGING_CURRENT acts as if the charging current sensor always reported -1.0A. That affects the value reported to ROS, the status LED, but also checkShouldCharge(). Since -1.0 will be lower than any configured i_charge_cutoff, this will never prevent charging.

I have no personal interest here. Again, I'm just looking at how this PR can be utilized, and since you asked for more settings, that was one that came to my mind. (Another one would be settings for for #97, but since that isn't merged yet, we can also add them there.)

From risk perspective, I guess it's no different from setting i_charge_cutoff to a huge value (or even Inf)?!? Except that IGNORE_CHARGING_CURRENT also makes sure that no random values are sent to ROS, which might be confusing in the UI. I think that was the "better no values than wrong values" part.

Apehaenger commented 1 month ago

I have no personal interest here. Again, I'm just looking at how this PR can be utilized, and since you asked for more settings, that was one that came to my mind. (Another one would be settings for for #97, but since that isn't merged yet, we can also add them there.)

Fully clear and I'm grateful for getting such a detailed review and good hints!! Regarding #97: Now that we try to have nothing unrelated in this PR, lets go on this way, even if it would be more clever to add SHUTDOWN_ESC_MAX_PITCH now, because the one who add that, has also to decide for the num of hall spares :-/

From risk perspective, I guess it's no different from setting i_charge_cutoff to a huge value (or even Inf)?!? Except that IGNORE_CHARGING_CURRENT also makes sure that no random values are sent to ROS, which might be confusing in the UI. I think that was the "better no values than wrong values" part.

Yes, and that time, my reasoning for moving the IGNORE implementation to checkShouldCharge() was the opposite, because a couple of ppl. use this IGNORE thingy for an increased battery charging over the fixed 1.5A and don't see if they charge with 2A or 3A. But that should be solved soon ;-)

Apehaenger commented 1 month ago

Time for an intermediate result. Tested the protocol together with ROS and made some alignments, nothing more except that I also implemented the last discussed stuff as well as that I added shutdown_esc_max_pitch config member. It get merged earlier or later anyway ;-)

Next, I'll check if LittleFS is working as expected.

ROS side's PR https://github.com/ClemensElflein/open_mower_ros/pull/163

Apehaenger commented 1 month ago

Please see here https://github.com/ClemensElflein/open_mower_ros/pull/163#issuecomment-2423292585 about state/description

Apehaenger commented 1 month ago

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 (also from @ClemensElflein) before asking for tester @ discord.

karlranseierausrom commented 1 month ago

May I already test? Is there an easy way to change the firmware version? I would like to check voltage and current settings to fit to my bigger battery.

Apehaenger commented 1 month ago

May I already test? Is there an easy way to change the firmware version? I would like to check voltage and current settings to fit to my bigger battery.

Sure, testing is always welcome :innocent:

Pico-FW is here: https://github.com/ClemensElflein/OpenMower/actions/runs/11534325476/artifacts/2107987830 (if you don't know how to flash it, pls. contact me on discord)

ROS Image via: sudo podman pull ghcr.io/clemenselflein/open_mower_ros:pr-163 (don't forget to change your /boot/openmower/openmower_version.txt to pr-163)

Please check the sample config file regarding the required parameter: https://github.com/ClemensElflein/open_mower_ros/blob/38396194a4cc5d71de48d489fd35c19ca52f929a/config/mower_config.sh.example#L197-L210

You may watch the ll_high_level_config word in the openmower-log to follow up the configuration.

ClemensElflein / OpenMower