[automower] Add planner, calendar and command channels #8802

marcinczeczko · 2020-10-19T10:52:15Z

I'd like to propose couple of improvements to the Automower binding:
- Added support for the planner and calendar data
- Added command channels
- Removed name channel
- Updated docs

Signed-off-by: Marcin Czeczko marcin.czeczko@gmail.com

TravisBuddy · 2020-10-19T10:57:16Z

Travis tests were successful

Hey @marcinczeczko,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

fwolter · 2020-10-23T17:13:51Z

@maxpg Can you take a look?

maxpg · 2020-11-06T13:40:11Z

@fwolter Sorry for the delay, pretty busy currently. I try to have a look this weekend...

maxpg

Thanks for your contribution. Definitely a major improvement to the binding. Also in the area of documentation.
Unfortunately I am not able to test the changes as my automower has already ended its season...

maxpg · 2020-11-08T19:38:19Z

....automower/src/main/java/org/openhab/binding/automower/internal/things/AutomowerHandler.java

@@ -89,9 +110,24 @@ public AutomowerHandler(Thing thing) {

    @Override
    public void handleCommand(ChannelUID channelUID, Command command) {
-        if (command instanceof RefreshType) {
+        Optional<AutomowerCommand> commandName = AutomowerCommand.fromChannelUID(channelUID);


As you don't do anything with the AutomowerCommand in case the command is a refresh I would fetch the command in the else clause of the refresh check...

@marcinczeczko can you comment on this?

Yep, it's a good idea. Will fix that

Good point. Will update

maxpg · 2020-11-08T19:40:34Z

....automower/src/main/java/org/openhab/binding/automower/internal/things/AutomowerHandler.java

-                    new DateTimeType(ZonedDateTime.ofInstant(errorCodeTimestamp, ZoneId.systemDefault())));
+            long errorCodeTimestamp = mower.getAttributes().getMower().getErrorCodeTimestamp();
+            if (errorCodeTimestamp == 0L) {
+                updateState(CHANNEL_STATUS_ERROR_TIMESTAMP, UnDefType.NULL);


Thanks for fixing the problem with the error timestamp :-)

No problem :)

maxpg · 2020-11-08T19:43:27Z

bundles/org.openhab.binding.automower/README.md

+| mower-status#planner-next-start      | DateTime | R | The time for the next auto start. If the mower is charging then the value is the estimated time when it will be leaving the charging station. If the mower is about to start now, the value is NULL.                                                                  |
+| mower-status#planner-override-action | String   | R | The action that overrides current planner operation.                                                                                                                                                                                                                  |
+| mower-status#calendar-tasks          | String   | R | The JSON with the information about Automower planner.                                                                                                    |
+| mower#start                       | Number   | W | Starts the automower for a duration                     |


Cool, I like the new command channels. I tried to also do command channels in my initial contribution but they were rejected. But probably because of the way that I implemented them, they didn't fit well and were more like a workaround because I wanted to use actions.

fwolter

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

fwolter · 2020-11-19T20:18:51Z

....automower/src/main/java/org/openhab/binding/automower/internal/things/AutomowerHandler.java

@@ -89,9 +110,24 @@ public AutomowerHandler(Thing thing) {

    @Override
    public void handleCommand(ChannelUID channelUID, Command command) {
-        if (command instanceof RefreshType) {
+        Optional<AutomowerCommand> commandName = AutomowerCommand.fromChannelUID(channelUID);


@marcinczeczko can you comment on this?

fwolter · 2020-11-19T20:27:18Z

bundles/org.openhab.binding.automower/src/main/resources/OH-INF/thing/thing-types.xml

-		<channels>
-			<channel id="name" typeId="nameType"/>
-			<channel id="mode" typeId="modeType"/>
-			<channel id="activity" typeId="activityType"/>
-			<channel id="state" typeId="stateType"/>
-			<channel id="last-update" typeId="lastUpdateType"/>
-			<channel id="battery" typeId="batteryType"/>
-			<channel id="error-code" typeId="errorCodeType"/>
-			<channel id="error-timestamp" typeId="errorTimestampType"/>
-		</channels>
+		<channel-groups>
+			<channel-group id="mower-status" typeId="mowerStatus"/>
+			<channel-group id="mower" typeId="mowerCommands"/>
+		</channel-groups>


The grouping of the Channels makes the binding incompatible with current configurations. That means existing users need to remove their mower Thing, re-add it and adjust their Items configuration. Although this breaking change is allowed during the OH 2.5.x -> 3.0.0 release, I think it would annoy users. Is this change really worth it?

Adding new Channels without touching existing ones is okay.

Thanks for that comment. I absolutely agree with that. Will update it accordingly

bundles/org.openhab.binding.automower/src/main/resources/OH-INF/thing/thing-types.xml

fwolter · 2021-01-13T22:33:00Z

@marcinczeczko What is the status of this PR?

marcinczeczko · 2021-02-25T14:18:45Z

@fwolter - Sorry for not responding for long time. Lot of things were happening on my side that simple blocked myself from any contributions. But fortunately it's getting calm now and I plan to resurrect my PR

marcinczeczko · 2021-02-25T22:05:55Z

I addressed all review comments in the latest commit.

fwolter

You need to sign-off your commits. See https://www.openhab.org/docs/developer/contributing.html#sign-your-work The necessary commands are listed when you click on "Details" at the DCO check below.

fwolter · 2021-03-02T21:19:24Z

....automower/src/main/java/org/openhab/binding/automower/internal/things/AutomowerHandler.java

+        ZonedDateTime gmt = ZonedDateTime.ofInstant(Instant.ofEpochMilli(timestamp), ZoneOffset.UTC);
+        return ZonedDateTime.of(gmt.getYear(), gmt.getMonthValue(), gmt.getDayOfMonth(), gmt.getHour(), gmt.getMinute(),
+                gmt.getSecond(), gmt.getNano(), ZoneId.systemDefault());


The user can set an individual timezone in OH's settings. You can obtain it by injecting TimeZoneProvider.

Suggested change

ZonedDateTime gmt = ZonedDateTime.ofInstant(Instant.ofEpochMilli(timestamp), ZoneOffset.UTC);

return ZonedDateTime.of(gmt.getYear(), gmt.getMonthValue(), gmt.getDayOfMonth(), gmt.getHour(), gmt.getMinute(),

gmt.getSecond(), gmt.getNano(), ZoneId.systemDefault());

LocalDateTime local = LocalDateTime.ofInstant(Instant.ofEpochMilli(timestamp), ZoneOffset.UTC);

return ZonedDateTime.of(local, timeZoneProvider.getTimeZone());

fwolter · 2021-03-02T21:20:22Z

bundles/org.openhab.binding.automower/src/main/resources/OH-INF/thing/thing-types.xml

+
+	<channel-type id="plannerOverrideActionType">
+		<item-type>String</item-type>
+		<label>The override action</label>


Words in labels should be capitalized (except prepositions and so on). See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions

Suggested change

<label>The override action</label>

<label>Override Action</label>

fixed labels

fwolter · 2021-03-02T21:20:50Z

bundles/org.openhab.binding.automower/src/main/resources/OH-INF/thing/thing-types.xml

+
+	<channel-type id="calendarTasksType">
+		<item-type>String</item-type>
+		<label>The information about the planner as JSON</label>


Labels are expected to be as short as possible. Guideline is 2-3 words with max 20-25 chars. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions

- Added support for the planner and calendar data - Added command channels - Updated docs Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com>

marcinczeczko · 2021-03-03T10:30:10Z

I pushed one more commit with the last changes.

groups. Improved the mower state update: - Cache the last read state from API - Use cached mower state so the items linked will always be up to date without the need to wait for API refresh period. - Use timeZoneProvider to user user set timezone. Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com>

fwolter

You could re-request another review when you are finished making all changes. I will get notified, then.

fwolter · 2021-03-27T15:11:25Z

....automower/src/main/java/org/openhab/binding/automower/internal/things/AutomowerHandler.java

 */
-@NonNullByDefault


Did you remove this by intention?

I think I did it intentionally that time due to the new class field mowerState but apparently I forgot to clean it up. Already fixed it.

marcinczeczko · 2021-03-30T21:30:20Z

You could re-request another review when you are finished making all changes. I will get notified, then.

Sorry I didn't know about re-request option :)

Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com>

fwolter

LGTM

* [Automower] Enhanced binding: - Added support for the planner and calendar data - Added command channels - Updated docs Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com> * [Automower] Fixed consts with channel ids after removal of channel groups. Improved the mower state update: - Cache the last read state from API - Use cached mower state so the items linked will always be up to date without the need to wait for API refresh period. - Use timeZoneProvider to user user set timezone. Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com> * Rolledback NotNullByDefault annotation Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com>

* [Automower] Enhanced binding: - Added support for the planner and calendar data - Added command channels - Updated docs Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com> * [Automower] Fixed consts with channel ids after removal of channel groups. Improved the mower state update: - Cache the last read state from API - Use cached mower state so the items linked will always be up to date without the need to wait for API refresh period. - Use timeZoneProvider to user user set timezone. Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com> * Rolledback NotNullByDefault annotation Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com> Signed-off-by: John Marshall <john.marshall.au@gmail.com>

* [Automower] Enhanced binding: - Added support for the planner and calendar data - Added command channels - Updated docs Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com> * [Automower] Fixed consts with channel ids after removal of channel groups. Improved the mower state update: - Cache the last read state from API - Use cached mower state so the items linked will always be up to date without the need to wait for API refresh period. - Use timeZoneProvider to user user set timezone. Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com> * Rolledback NotNullByDefault annotation Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com>

openhab-bot · 2024-09-14T09:05:22Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/automower-missing-channels/122319/4

cpmeister added the enhancement An enhancement or new feature for an existing add-on label Oct 24, 2020

maxpg approved these changes Nov 8, 2020

View reviewed changes

fwolter requested changes Nov 19, 2020

View reviewed changes

fwolter added the awaiting feedback Awaiting feedback from the pull request author label Dec 6, 2020

marcinczeczko force-pushed the automower-updates branch from baaf89b to b036272 Compare February 25, 2021 22:05

fwolter removed the awaiting feedback Awaiting feedback from the pull request author label Mar 2, 2021

fwolter requested changes Mar 2, 2021

View reviewed changes

[Automower] Enhanced binding:

46a057e

- Added support for the planner and calendar data - Added command channels - Updated docs Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com>

marcinczeczko force-pushed the automower-updates branch from b036272 to 46a057e Compare March 3, 2021 09:00

marcinczeczko force-pushed the automower-updates branch from a831fc7 to fb14699 Compare March 3, 2021 10:33

marcinczeczko force-pushed the automower-updates branch from fb14699 to 75303ff Compare March 4, 2021 07:36

fwolter requested changes Mar 27, 2021

View reviewed changes

marcinczeczko requested a review from fwolter March 30, 2021 22:15

Rolledback NotNullByDefault annotation

0a3ef6c

Signed-off-by: Marcin Czeczko <marcin.czeczko@gmail.com>

marcinczeczko force-pushed the automower-updates branch from e766641 to 0a3ef6c Compare March 30, 2021 22:16

fwolter approved these changes Mar 31, 2021

View reviewed changes

fwolter changed the title ~~[Automower] Enhanced binding~~ [automower] Add planner, calendar and command channels Mar 31, 2021

fwolter merged commit 0eb48e6 into openhab:main Mar 31, 2021

fwolter added this to the 3.1 milestone Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[automower] Add planner, calendar and command channels #8802

[automower] Add planner, calendar and command channels #8802

marcinczeczko commented Oct 19, 2020

TravisBuddy commented Oct 19, 2020

fwolter commented Oct 23, 2020

maxpg commented Nov 6, 2020

maxpg left a comment

maxpg Nov 8, 2020

fwolter Nov 19, 2020

marcinczeczko Feb 25, 2021

marcinczeczko Feb 25, 2021

maxpg Nov 8, 2020

marcinczeczko Feb 25, 2021

maxpg Nov 8, 2020

fwolter left a comment

fwolter Nov 19, 2020

fwolter Nov 19, 2020 •

edited

Loading

marcinczeczko Feb 25, 2021

fwolter commented Jan 13, 2021

marcinczeczko commented Feb 25, 2021

marcinczeczko commented Feb 25, 2021

fwolter left a comment

fwolter Mar 2, 2021

marcinczeczko Mar 3, 2021

fwolter Mar 2, 2021

marcinczeczko Mar 3, 2021

fwolter Mar 2, 2021

marcinczeczko Mar 3, 2021

marcinczeczko commented Mar 3, 2021

fwolter left a comment

fwolter Mar 27, 2021

marcinczeczko Mar 30, 2021

marcinczeczko commented Mar 30, 2021

fwolter left a comment

openhab-bot commented Sep 14, 2024

	<label>The override action</label>
	<label>Override Action</label>

		*/
		@NonNullByDefault

[automower] Add planner, calendar and command channels #8802

[automower] Add planner, calendar and command channels #8802

Conversation

marcinczeczko commented Oct 19, 2020

TravisBuddy commented Oct 19, 2020

Travis tests were successful

fwolter commented Oct 23, 2020

maxpg commented Nov 6, 2020

maxpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwolter Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwolter commented Jan 13, 2021

marcinczeczko commented Feb 25, 2021

marcinczeczko commented Feb 25, 2021

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinczeczko commented Mar 3, 2021

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinczeczko commented Mar 30, 2021

fwolter left a comment

Choose a reason for hiding this comment

openhab-bot commented Sep 14, 2024

fwolter Nov 19, 2020 •

edited

Loading