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.

Sign up for GitHub

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

Jump to bottom

[somneo] Add alarm support and other improvements #14882

Merged
merged 17 commits into from
Jun 17, 2023
Merged

[somneo] Add alarm support and other improvements #14882

merged 17 commits into from
Jun 17, 2023

Conversation

myrck
Copy link
Contributor

@myrck myrck commented Apr 25, 2023

Description

  • Add support to control all the alarm clocks of the device
  • Fix a issue that some channels used the wrong State class

Testing

org.openhab.binding.somneo-3.4.4-SNAPSHOT.jar
org.openhab.binding.somneo-4.0.0-SNAPSHOT.jar

myrck added 6 commits April 25, 2023 21:02
@myrck
[somneo] Add alarm clock channels
def21d9 
Signed-off-by: Michael Myrcik <[email protected]>
@myrck
[somneo] Refactoring polling
452a099 
Signed-off-by: Michael Myrcik <[email protected]>
@myrck
[somneo] Fix wrong state for duration
f0be543 
Signed-off-by: Michael Myrcik <[email protected]>
@myrck
[somneo] Documentation
daa15a3 
Signed-off-by: Michael Myrcik <[email protected]>
@myrck
[somneo] Fix unused parameter
b149b61 
Signed-off-by: Michael Myrcik <[email protected]>
@myrck
[somneo} Fix possible null value
303ae8b 
Signed-off-by: Michael Myrcik <[email protected]>
@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/new-binding-philips-somneo-hf3670-connected-sleep-and-wake-up-light/133464/4

@jlaur jlaur changed the title [somneo] Add alarm support and other improvments [somneo] Add alarm support and other improvements Apr 25, 2023
@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Apr 25, 2023
clinique
clinique requested changes Apr 26, 2023
View reviewed changes
...binding.somneo/src/main/java/org/openhab/binding/somneo/internal/SomneoBindingConstants.java
@@ -58,6 +58,21 @@ public class SomneoBindingConstants {
public static final String CHANNEL_SUNSET_REMAINING_TIME = "sunset#remainingTime";
public static final String CHANNEL_SUNSET_SWITCH = "sunset#switch";
public static final String CHANNEL_SUNSET_VOLUME = "sunset#volume";
public static final String CHANNEL_ALARM_SNOOZE = "alarm#snooze";
Copy link
Contributor

Choose a reason for hiding this comment

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

Group id alarm should be separated from channel id snooze.
This applies to all these channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. What exactly do you mean by separate? Rename the group id alarm because it is too similar to the other alarm groups ids with an index (alarm{index})?

Copy link
Contributor

Choose a reason for hiding this comment

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

@clinique - can you help clarifying this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@clinique - gentle ping.

Copy link
Contributor

Choose a reason for hiding this comment

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

alarm is the group name, while snooze is the channel itself. You could directly target it using channelUID.getIdWithoutGroup(); in your handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For handling the commands it would be enough in most cases to check only the channel id. I would then have to divide the logic on several switchs, since not all ids are unique. The variables are also used for updateState(...). There I would have to build the id together with the group, or is only the channel id enough if it is unique? But then I would have to rebuild the id with the group for the channel ids that are not unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

@0x4d4d - did you see @clinique's reply?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlaur Sorry, I had commented but it had the pending label and don't saw it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any issues with the current approach that should block this PR longer.

@clinique - considering @0x4d4d's latest comment, please continue and conclude the discussion here and do a follow-up PR if there is anything that can be improved without introducing unnecessary complexity or redundancy. I have seen similar comments from you in other PR's, so I think it would be nice to reach a conclusion/consensus.

You are also welcome to have a look at #14376 where I have something similar and couldn't find a better solution:

https://github.com/openhab/openhab-addons/pull/14376/files#diff-002ecf5a3879dda71cf4fcb82dbfef3f550c0f1655cfb16134fa3e4f3b431d69R42-R54

...binding.somneo/src/main/java/org/openhab/binding/somneo/internal/SomneoBindingConstants.java
public static final String CHANNEL_ALARM_VOLUME = "alarm%d#volume";

// Regex for alarm channels
public static final String CHANNEL_ALARM_PREFIX_REGEX = "^alarm(\\d{1,2})#\\w+$";
Copy link
Contributor

Choose a reason for hiding this comment

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

Separating group id from channel id would avoid having to use a regex. This is provided directly by then channel id ( getIdWithoutGroup )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The regex is mainly needed to extract the alarm index from the group id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the regex with the use case in ok or is there something that needs to be done here?

....openhab.binding.somneo/src/main/java/org/openhab/binding/somneo/internal/SomneoHandler.java Outdated Show resolved Hide resolved
....openhab.binding.somneo/src/main/java/org/openhab/binding/somneo/internal/SomneoHandler.java Outdated Show resolved Hide resolved
...inding.somneo/src/main/java/org/openhab/binding/somneo/internal/model/AlarmSettingsData.java Outdated Show resolved Hide resolved
...inding.somneo/src/main/java/org/openhab/binding/somneo/internal/model/AlarmSettingsData.java Outdated Show resolved Hide resolved
jlaur
jlaur requested changes May 21, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! 👍 The code looks good, I have added only some minor comments.

bundles/org.openhab.binding.somneo/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.somneo/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.somneo/src/main/resources/OH-INF/thing/thing-types.xml Show resolved Hide resolved
....openhab.binding.somneo/src/main/java/org/openhab/binding/somneo/internal/SomneoHandler.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.somneo/src/main/resources/OH-INF/i18n/somneo_de.properties Outdated Show resolved Hide resolved
bundles/org.openhab.binding.somneo/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.somneo/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved
...binding.somneo/src/main/java/org/openhab/binding/somneo/internal/SomneoBindingConstants.java Outdated Show resolved Hide resolved
myrck and others added 3 commits May 22, 2023 20:30
@myrck @jlaur
[somneo] Improve description
3e02aeb 
Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: 0x4d4d <[email protected]>
@myrck @jlaur
[somneo] Improve label
bdbb005 
Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: 0x4d4d <[email protected]>
@myrck @jlaur
[somneo] Fix variable spelling
fe2b0fe 
Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: 0x4d4d <[email protected]>
@myrck myrck force-pushed the somneo-improvments branch from 8331d75 to fe2b0fe Compare May 22, 2023 19:05
@myrck myrck force-pushed the somneo-improvments branch from c584141 to 95c9676 Compare May 22, 2023 21:10
@myrck myrck force-pushed the somneo-improvments branch from f976c4b to a48282b Compare May 24, 2023 20:14
@myrck myrck requested a review from clinique June 1, 2023 07:35
jlaur
jlaur approved these changes Jun 17, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 17, 2023
@jlaur jlaur merged commit b505f7b into openhab:main Jun 17, 2023
@jlaur jlaur added this to the 4.0 milestone Jun 17, 2023
tb4jc pushed a commit to tb4jc/openhab-addons that referenced this pull request Jun 19, 2023
@myrck @tburri-alstom
[somneo] Add alarm support and other improvements (openhab#14882)
9772504 
* [somneo] Add alarm clock channels

Signed-off-by: Michael Myrcik <[email protected]>
Signed-off-by: Thomas Burri <[email protected]>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Aug 9, 2023
@myrck @matchews
[somneo] Add alarm support and other improvements (openhab#14882)
642b141 
* [somneo] Add alarm clock channels

Signed-off-by: Michael Myrcik <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
@myrck @austvik
[somneo] Add alarm support and other improvements (openhab#14882)
bfc2001 
* [somneo] Add alarm clock channels

Signed-off-by: Michael Myrcik <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur approved these changes

@clinique clinique Awaiting requested review from clinique

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@myrck @openhab-bot @clinique @jlaur