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

[automower] Implementation of complete automower API #17545

Merged
merged 40 commits into from
Feb 20, 2025

Conversation

MikeTheTux
Copy link
Contributor

@MikeTheTux MikeTheTux commented Oct 12, 2024

[automower] Implementation of complete automower API

References:

Current Status / TODOs:

  1. Implementation of Read Channels:
    1.1 DONE
  2. Implementation of Write Channels:
    2.1 DONE /mowers/{id}/actions
    2.2 DONE /mowers/{id}/calendar
    2.3 DONE /mowers/{id}/errors/confirm
    2.4 DONE /mowers/{id}/settings
    2.5 DONE /mowers/{id}/stayOutZones/{stayOutId}
    2.6 DONE /mowers/{id}/workAreas/{workAreaId}
    2.7 DONE /mowers/{id}/workAreas/{workAreaId}/calendar
    2.8 DONE /mowers/{id}/messages
  3. Dynamic Channels and Channel Groups
    3.1 DONE
  4. Readme
    4.1 DONE
  5. New Cut vs Search ratio channel:
    5.1 DONE
  6. openHAB actions
    6.1 DONE
  7. Better meta data (e.g. unit and state description) on channels
    7.1 DONE
  8. Fix [automower] planner-next-start does not reflect the correct offset to GMT #17147
    8.1 DONE
  9. i18n update
    9.1 DONE
  10. upgrade instructions
    10.1 DONE

@MikeTheTux MikeTheTux marked this pull request as draft October 12, 2024 06:44
@MikeTheTux MikeTheTux changed the title [automower] Implementation of complete automower API [automower] Implementation of complete automower API [WIP] Oct 12, 2024
@MikeTheTux MikeTheTux added the work in progress A PR that is not yet ready to be merged label Oct 12, 2024
@MikeTheTux
Copy link
Contributor Author

@lsiepel, do you have a good example how to implement dynamic Channels and Channel Groups? Esp. dynamic Channel Groups seems to be tricky.

@MikeTheTux MikeTheTux added the enhancement An enhancement or new feature for an existing add-on label Oct 12, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Oct 12, 2024

@lsiepel, do you have a good example how to implement dynamic Channels and Channel Groups? Esp. dynamic Channel Groups seems to be tricky.

Interesting combination, don't have an example straight out of my head, let me check.

@MikeTheTux
Copy link
Contributor Author

Except from dynamic channels and channel groups (which is an usability improvement only), I would consider the main part of the implementation is done.
I think it would be great that we bring this complex PR into the December release (which should be the winter break of the mower in the northern hemisphere). Therefore I would like to request an initial review round.

@MikeTheTux MikeTheTux requested a review from lsiepel October 30, 2024 15:25
@MikeTheTux MikeTheTux removed the work in progress A PR that is not yet ready to be merged label Nov 9, 2024
@MikeTheTux MikeTheTux changed the title [automower] Implementation of complete automower API [WIP] [automower] Implementation of complete automower API Nov 9, 2024
@MikeTheTux MikeTheTux requested review from wborn and jlaur November 10, 2024 18:40
@MikeTheTux MikeTheTux marked this pull request as ready for review November 14, 2024 18:07
@MikeTheTux
Copy link
Contributor Author

Implementation done - ready for review

@MikeTheTux
Copy link
Contributor Author

MikeTheTux commented Dec 6, 2024

In the meanwhile also dynamic channels are implemented.

The issue reported in openhab/openhab-webui#575 seems to be not fixed, when the channels are generated automatically. As workaround I'm adding dynamic Channel Labels as well. The Labels are then used to create unique channel uids:

newChannel = ChannelBuilder.create(channelUid, itemType).withType(channelTypeUID).withLabel(label).build();

@lsiepel, Is it worth creating an issue?

@MikeTheTux
Copy link
Contributor Author

@lsiepel, will you find time to review this major change set as well? thanks in advance!

@lsiepel
Copy link
Contributor

lsiepel commented Dec 21, 2024

The issue reported in openhab/openhab-webui#575 seems to be not fixed, when the channels are generated automatically. As workaround I'm adding dynamic Channel Labels as well. The Labels are then used to create unique channel uids

Not sure if i understand the problem here. You should make sure the channelUID is unique, The label is just for display and while it woudl be very usefull, it is not constraint to be unique.

will you find time to review this major change set as well? thanks in advance!
Yes, i have holidays now, so might review this monday or tuesday, but if someone else has spare time, don't wait for me.

In the meantime, could you fix the conflict?

@MikeTheTux
Copy link
Contributor Author

MikeTheTux commented Dec 22, 2024

The issue reported in openhab/openhab-webui#575 seems to be not fixed, when the channels are generated automatically. As workaround I'm adding dynamic Channel Labels as well. The Labels are then used to create unique channel uids

Not sure if i understand the problem here. You should make sure the channelUID is unique, The label is just for display and while it woudl be very usefull, it is not constraint to be unique.

The channelUID is already unique due to the running index. The Label was not unique and repeating per message (channel.label was not set, channelType.label set, but the same type is used for all channels - therefore not unique)
This caused that the channel-wizard proposed Items using the same item name. As the item name is derived from the label, I added now the running index to the label as well ...

I think this is the code how the item names are proposed:
openhab-webui/blob/main/bundles/org.openhab.ui/web/src/components/thing/channel-list.vue

        let newItemName = this.newItemsPrefix || this.$oh.utils.normalizeLabel(this.thing.label)
        newItemName += '_'
        let suffix = channel.label || channelType.label || channel.id

It is using the thing.label + channel.label (if available), fallback to channelType.label, fallback to channel.id

As an improvement we could request that openHAB UI implements a duplication check at this stage.

Nevertheless the code works as expected / intended.
In the meanwhile I even think that adding the running index to the label makes sense. Otherwise we would produce 50 items using the same description and the user can differ them only via the channelID (but not the label).

This is how the current implementation using the workaround looks like:
image

I'm happy with that implementation in the binding. No need to change.

@aMU5Ed
Copy link

aMU5Ed commented Feb 16, 2025

In December I got an email from Husqvarna that there will be changes in the APIs and the backend. Has anyone tested the binding with this new version? Old version will only be available until 28th of February 2025.

@MikeTheTux
Copy link
Contributor Author

In December I got an email from Husqvarna that there will be changes in the APIs and the backend. Has anyone tested the binding with this new version? Old version will only be available until 28th of February 2025.

This change will affect Automower Connect Websocket API (new v2). The Automower Connect REST API will stay the same. This binding is using REST API only. Therefore it's not affected.

@lsiepel
Copy link
Contributor

lsiepel commented Feb 17, 2025

Could you fix the conflixct?

@MikeTheTux
Copy link
Contributor Author

Could you fix the conflixct?

#18061 on main is the reason for the conflict. What is the best approach with least effort to resolve the conflict?

@lsiepel
Copy link
Contributor

lsiepel commented Feb 18, 2025

Could you fix the conflixct?

#18061 on main is the reason for the conflict. What is the best approach with least effort to resolve the conflict?

I would cascade all upstream commits: update your main from openhab main, afterwards update your feature branch from your main etc. Eventually your IDE will show you the conflict and ask for resolving it. If it's only the license header it would be easy to fix.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Just a minor comment. As this is an extensive PR, i would like to double check that

  1. this is very well tested?
  2. the update instructions can't fix everything, i think an upgrade notice is mandatory.

Comment on lines 125 to 159
<!-- optional channels
<add-channel id="<xx>-start" groupIds="calendartasks">
<type>automower:calendarTasksStartType</type>
</add-channel>
<add-channel id="<xx>-duration" groupIds="calendartasks">
<type>automower:calendarTasksDurationType</type>
</add-channel>
<add-channel id="<xx>-monday" groupIds="calendartasks">
<type>automower:calendarTasksMondayType</type>
</add-channel>
<add-channel id="<xx>-tuesday" groupIds="calendartasks">
<type>automower:calendarTasksTuesdayType</type>
</add-channel>
<add-channel id="<xx>-wednesday" groupIds="calendartasks">
<type>automower:calendarTasksWednesdayType</type>
</add-channel>
<add-channel id="<xx>-thursday" groupIds="calendartasks">
<type>automower:calendarTasksThursdayType</type>
</add-channel>
<add-channel id="<xx>-friday" groupIds="calendartasks">
<type>automower:calendarTasksFridayType</type>
</add-channel>
<add-channel id="<xx>-saturday" groupIds="calendartasks">
<type>automower:calendarTasksSaturdayType</type>
</add-channel>
<add-channel id="<xx>-sunday" groupIds="calendartasks">
<type>automower:calendarTasksSundayType</type>
</add-channel>
<add-channel id="<xx>-workAreaId" groupIds="calendartasks">
<type>automower:calendarTasksWorkAreaIdType</type>
</add-channel>
<add-channel id="<xx>-workArea" groupIds="calendartasks">
<type>automower:workareaNameType</type>
</add-channel>
-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Use er remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed channels that are dynamically handled by the binding and therefore not specified in the update instruction.

Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
set several channels to advance
added update.xml

Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
added poll channels
tests and smaller fixes
creating all channels at once due to performance issues

Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: Michael Weger <weger.michael@gmx.net>
@lsiepel
Copy link
Contributor

lsiepel commented Feb 19, 2025

Only the failed build due to the factory not adding the timezoneprovider to the handler constructor.

updating header to 2025 for new files

Signed-off-by: Michael Weger <weger.michael@gmx.net>
@MikeTheTux
Copy link
Contributor Author

Only the failed build due to the factory not adding the timezoneprovider to the handler constructor.

Slowly but surely ...

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.
Do you need additional time to test the latest changes?

As this is a breaking change and the Thing needs to be recreated to have everything setup right, i would like to ask you to create an upgrade notice in the distro repository.

}

@Override
public void handleCommand(ChannelUID channelUID, Command command) {
if (RefreshType.REFRESH == command) {
logger.debug("Refreshing channel '{}'", channelUID);
refreshChannels(channelUID);
// refreshChannels(channelUID); // causes >100 channel updates during setup
Copy link
Contributor

Choose a reason for hiding this comment

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

Left by mistake? Log says "Refreshing channel", so something seems not fully right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the logger as well as the commented refreshChannels call:

        if (RefreshType.REFRESH == command) {
            // not implemented as it would causes >100 channel updates in a row during setup (performance)
        } else {

@MikeTheTux
Copy link
Contributor Author

The initial implementation was tested quite allot and I'm also using it since then w/o issue.
Nevertheless I will work in the latest findings and then do some testing again before we move forward.

Is there a known deadline to meet for the next (5.0.0) release?
I'm quite busy within the next days an even weeks. Not sure, how fast I'm able to progress.

@lsiepel
Copy link
Contributor

lsiepel commented Feb 20, 2025

ess I will work in the latest findings and then do some testing again before we move forward.

Is there a known deadline to meet for the next (5.0.0) release? I'm quite busy within the next days an even weeks. Not sure, how fast I'm able to progress.

Deadline will be somewhere at the end of june i guess. But prefereably we add it before in a milestone to get some mileage and be sure all is stable before the mayor release. No rush, we have plenty of time.

@MikeTheTux
Copy link
Contributor Author

As this is a breaking change and the Thing needs to be recreated to have everything setup right, i would like to ask you to create an upgrade notice in the distro repository.

The thing must not be re-created (remove and re-add thing). But new channels have to linked to items (if they shall be used). I will create an upgrade notice for that.

Signed-off-by: Michael Weger <weger.michael@gmx.net>
@MikeTheTux
Copy link
Contributor Author

As this is a breaking change and the Thing needs to be recreated to have everything setup right, i would like to ask you to create an upgrade notice in the distro repository.

The thing must not be re-created (remove and re-add thing). But new channels have to linked to items (if they shall be used). I will create an upgrade notice for that.

done

@lsiepel lsiepel merged commit 4ce97d5 into openhab:main Feb 20, 2025
2 checks passed
@lsiepel lsiepel added this to the 5.0 milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[automower] planner-next-start does not reflect the correct offset to GMT
4 participants