Sabo WIP

[Apehaenger]

Just my Sabo related changes for not blocking possible larger structural changes.

Will spam commit all my changes to here, but only if changes got tested before.

[ClemensElflein]

we should also be checking for xcore 1.x.

[Apehaenger]

Think this could be merged as it runs since a couple of months on my mower.

But I'm unsure with the last two src/services/mower_ui_service/mower_ui_service.* files. So... would feel better if you could take a quick look before merge.

[rovo89]

But I'm unsure with the last two src/services/mower_ui_service/mower_ui_service.* files. So... would feel better if you could take a quick look before merge.

Was that the service that I requested to be renamed to "high-level service"? I had completely reworked #17, which I think your PR is based on.

[rovo89]

You didn't integrate your inputs into the input service yet, did you? See https://github.com/xtech/fw-openmower-v2/pull/17/files#diff-cdba987ed1e89d913f0b4969701dac9b177515ba67391c76da8cc0684c127da1R208-R223 for YF, although all of my work there was just theoretical.

[Apehaenger]

I've no idea 😇 ... to long ago.
If I remember right I copied it from somewhere half finished, when I tried to get high-level states and probably button-actions for my Sabo-CoverUI.

[rovo89]

f6c6075

[rovo89]

Indeed, that was a while ago, also the discussion around it... but I remember that I spent quite some time rewriting the YF branch. Too bad that it isn't merged yet and I think not even tested (or did you try it, @ClemensElflein?). Also not sure who uses YF with v2, i.e. if we'd disturb someone's working state if we just merged it, as base for this PR.

[rovo89]

Or maybe merge this one, just a single commit to add the High Level Service:
main...feature/high-level-service

The YF branch makes only small amends to that service, some more for the input service (but I think you're not using that yet anyway).

[rovo89]

Quickly searched your changes - to you even use the mower_ui_service? I only found references to it in commented out code. So maybe you can just remove the two files, then you'd only add new things (lvgl) and touch the Sabo stuff, so no impact expected on other vendors.

The only changes I'd like to learn more about are in main.cpp, will add a small review.

[ClemensElflein]

Indeed, that was a while ago, also the discussion around it... but I remember that I spent quite some time rewriting the YF branch. Too bad that it isn't merged yet and I think not even tested (or did you try it, @ClemensElflein?). Also not sure who uses YF with v2, i.e. if we'd disturb someone's working state if we just merged it, as base for this PR.

I have tried it, but honestly cannot remember. My parents are not using the v2 mower because they claim it has worse GPS reception (which I cannot believe - since we've not changed anything on the NTRIP), so its not really being tested from my side.

[rovo89]

so its not really being tested from my side.

OK. The question is whether we can merge it anyway (after a bit more review), as I think it brings us a few steps forward. Not good to have too many pending changes. +824 -71 lines, so I don't think it will make things worse. Similar with v2, it shouldn't drift away.

[Apehaenger]

You didn't integrate your inputs into the input service yet, did you? See https://github.com/xtech/fw-openmower-v2/pull/17/files#diff-cdba987ed1e89d913f0b4969701dac9b177515ba67391c76da8cc0684c127da1R208-R223 for YF, although all of my work there was just theoretical.

No, I don't have those packets because my CoverUI buttons are SPI driven.
Also, I planned to handle my buttons within the Firmware because I need to track them in LVGL for menu navigation and the like. Could be, that I will send virtual buttons at the end or even have buttons with direct high-level functionality like Start/Stop/Pause.

However can try to implement later, but need to get forward somehow and not get pushed backward 😇

These early months have been pretty chaotic 😇 Because it was told: Pull this, merge that, copy from this file... at the end I fully lost the overview. I'm surprised that the code is mergeable at all 👍

[rovo89]

However can try to implement later, but need to get forward somehow and not get pushed backward 😇

Yes - my intention was that you can merge this very soon, and the fact that you haven't integrated it with the input service helps in this case. I think if you just remove the mower_ui_service files (which aren't even in CMakeLists.txt), it should be good.

[Apehaenger]

Or maybe merge this one, just a single commit to add the High Level Service: main...feature/high-level-service

The YF branch makes only small amends to that service, some more for the input service (but I think you're not using that yet anyway).

Oh cool! Will that commit mean that I get the high-level states? Or is it only some kind of preparation for future implementation?
If not, I could go on with my CoverUI work 😄

[Apehaenger]

Quickly searched your changes - to you even use the mower_ui_service? I only found references to it in commented out code. So maybe you can just remove the two files, then you'd only add new things (lvgl) and touch the Sabo stuff, so no impact expected on other vendors.

The only changes I'd like to learn more about are in main.cpp, will add a small review.

Indeed, you're right, the files even don't get compiled 👍
Quite thanks, removed them.

[Apehaenger]

However can try to implement later, but need to get forward somehow and not get pushed backward 😇

Yes - my intention was that you can merge this very soon, and the fact that you haven't integrated it with the input service helps in this case. I think if you just remove the mower_ui_service files (which aren't even in CMakeLists.txt), it should be good.

Quite thanks 👍 Think I got it... I'm simply not that quick to understand and react 😇

[rovo89]

Oh cool! Will that commit mean that I get the high-level states? Or is it only some kind of preparation for future implementation?

I don't remember an implementation on ROS side... I think I just took what was there as "Mower UI service", renamed it and maybe cleaned it up a bit.

Anyhow - I would be fine if you want to hit "Squash and merge" now (once the checks have finished successfully).