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

[amazonechocontrol] Improve color temperature channel #17754

Merged
merged 20 commits into from
Nov 24, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Nov 17, 2024

Refers to #17638 (comment)

  • AmazonEcho => changing channel type to system:color-temperature-abs
  • AVMFritz => adding unitHint
  • Dali => nothing to change, already uses system:color-temperature-abs
  • Deconz => changing channel type to system:color-temperature-abs
  • Dmx => nothing to change, the value is not directly a Kelvin or mirek value
  • Epson projector => nothing to change, the value is not directly a Kelvin or mirek value
  • Feican => nothing to change, the value is not directly a Kelvin or mirek value
  • Govee => adding unitHint
  • LG TV serial => nothing to change, the value is not directly a Kelvin or mirek value
  • Lifx => nothing to change, already uses system:color-temperature-abs with dynamic state provider
  • Hue => nothing to change, already uses system:color-temperature-abs with dynamic state provider
  • MiHome => channel 'Dimmer' type but has state unit Kelvin; this seems rather mixed up; so no changes in this PR
  • MiLight => nothing to change, the value is not directly a Kelvin or mirek value
  • MQTT:EspMiLightBulb => nothing to change, already uses system:color-temperature-abs with dynamic state provider
  • MQTT:HomeAssistant => wip in [mqtt.homeassistant] Provide dynamic channel types for Sensor and Number with proper units #17651
  • MQTT:Homie => done in [mqtt.homie] Populate unit hint on dynamic channels #17649
  • NanoLeaf => nothing to change, already uses system:color-temperature-abs with dynamic state provider
  • Samsung TV => nothing to change, the value is not directly a Kelvin or mirek value
  • Smart things => nothing to change, the value is not directly a Kelvin or mirek value
  • Sony projector => nothing to change, the color temperature is a list of discrete values
  • Tapo => changing channel type to system:color-temperature-abs plus dynamic state provider
  • TPLink => changing channel type to system:color-temperature-abs plus dynamic state provider
  • Tradfri => nothing to change, already uses system:color-temperature-abs
  • Yeelight => nothing to change, the value is not directly a Kelvin or mirek value
  • Zigbee => nothing to change, already uses system:color-temperature-abs with dynamic state provider
  • ZWave => nothing to change, the value is not directly a Kelvin or mirek value
  • ZWay => nothing to change, the value is not directly a Kelvin or mirek value

Signed-off-by: AndrewFG [email protected]

@andrewfg andrewfg added enhancement An enhancement or new feature for an existing add-on work in progress A PR that is not yet ready to be merged labels Nov 17, 2024
@andrewfg andrewfg self-assigned this Nov 17, 2024
@andrewfg andrewfg requested a review from lolodomo November 17, 2024 16:25
@andrewfg andrewfg added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Nov 17, 2024
Signed-off-by: AndrewFG <[email protected]>
@andrewfg andrewfg requested a review from mgeramb as a code owner November 18, 2024 12:30
Signed-off-by: AndrewFG <[email protected]>
Signed-off-by: AndrewFG <[email protected]>
@andrewfg andrewfg requested a review from J-N-K as a code owner November 18, 2024 13:05
Signed-off-by: AndrewFG <[email protected]>
Signed-off-by: AndrewFG <[email protected]>
@andrewfg andrewfg requested a review from Hilbrand as a code owner November 18, 2024 14:14
@andrewfg andrewfg added the rebuild Triggers Jenkins PR build label Nov 18, 2024
@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 18, 2024

@wildcs @pboos @BobRak @ccutrer for info and feedback

@ccutrer
Copy link
Contributor

ccutrer commented Nov 18, 2024

@andrewfg andrewfg removed the rebuild Triggers Jenkins PR build label Nov 18, 2024
@stefan-hoehn
Copy link
Contributor

I'd be happy to approve if I knew what the consequences of the hint are and where it could hurt or not and how I can test it. Any hint (no pun intended ;-) )?

@ccutrer
Copy link
Contributor

ccutrer commented Nov 18, 2024

A unitHint on a channel means when you go to that channel in MainUI and link it to a new item, that item will default to the channel's unitHint for its unit.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 18, 2024

This PR relates particulary to colour temperature channels on lights, which should have items that display in Kelvin or Mirek (micro inverse Kelvin). The problem is that currently OH creates such items by default using the local system temperature unit Celsius or Fahrenheit; neither of which is appropriate to colour temperatures..

@andrewfg andrewfg removed the work in progress A PR that is not yet ready to be merged label Nov 19, 2024
@andrewfg
Copy link
Contributor Author

@lolodomo I am not sure if tapocontrol and tplink require update instructions..

@andrewfg andrewfg added the rebuild Triggers Jenkins PR build label Nov 20, 2024
@andrewfg andrewfg changed the title Add unitHint for color temperature channels [AmazonEchoControl] Improvements to color temperature channel Nov 21, 2024
@andrewfg
Copy link
Contributor Author

@lolodomo I have split all the bindings into separate PRs. This PR is now repurposed for AmazonEchoControl only.

@andrewfg andrewfg changed the title [AmazonEchoControl] Improvements to color temperature channel [amazonechocontrol] Improvements to color temperature channel Nov 21, 2024
@andrewfg andrewfg requested a review from lolodomo November 21, 2024 18:12
@andrewfg andrewfg removed the work in progress A PR that is not yet ready to be merged label Nov 21, 2024
@lolodomo
Copy link
Contributor

@lolodomo I have split all the bindings into separate PRs. This PR is now repurposed for AmazonEchoControl only.

Please consider my review comments in the new PRs.

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

@jlaur jlaur changed the title [amazonechocontrol] Improvements to color temperature channel [amazonechocontrol] Improve color temperature channel Nov 21, 2024
@lolodomo
Copy link
Contributor

lolodomo commented Nov 23, 2024

When I create a managed thing (astro as an example), here is a portion of this thing in file org.openhab.core.thing.Thing.json:

{
  "astro:moon:local": {
    "class": "org.openhab.core.thing.internal.ThingStorageEntity",
    "value": {
      "isBridge": false,
      "channels": [
        {
          "uid": "astro:moon:local:rise#start",
          "id": "rise#start",
          "channelTypeUID": "astro:start",
          "itemType": "DateTime",
          "kind": "STATE",
          "label": "Heure Début",
          "description": "L\u0027heure de début de l\u0027événement",
          "defaultTags": [],
          "properties": {},
          "configuration": {
            "forceEvent": false,
            "offset": 0
          }
        },

So you can see that each channel has a field "itemType".
When you update the item type for a channel, please check that the upgrade tool is also updating the field "itemType" in file org.openhab.core.thing.Thing.json.
Either provide the new item type in the upgrade instructions in case it is supported. Or check if it is handled properly by the upgrade tool when using "update-channel". If not, try to use 2 instructions, one to remove the channel + one to add the channnel.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 23, 2024

The documentation of the thing upgrade tool is really hidden in our documentation. I am not able to find it from the menu or from a search.
Maybe @J-N-K who is the author of this upgrade tool can help us. I would like to know if item type can be provided in instructions or if by chance it is systematically updated.

@lolodomo
Copy link
Contributor

A Google search helped me.
Here is where it is documented:
https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

From the XSD, it looks like it is not possible to provide a new item type.

@andrewfg
Copy link
Contributor Author

From the XSD, it looks like it is not possible to provide a new item type.

Indeed!

That is why I was confused when you requested me to put that in the upgrade instructions.

I think there are two possible conclusions, either:

  1. If there is an upgrade instruction for a channel to a channel-type which has a different item-type, then OH core does already change the item-type automatically, or
  2. Somebody needs to open a PR to allow upgrading of the item-type.

I don't know if it is 1. or 2. above. But if it is 1. then I think my PRs are OK. Whereas if it is 2. then someone needs to open an issue and respective PR in OH Core; and we should mark my PRs as "pending other PR". Or ??

@lolodomo
Copy link
Contributor

lolodomo commented Nov 23, 2024

We need someone to test if item type is automatically updated when using "update-channel" instruction.

If not, try remove-channel + add-channel as a workaround. I am already almost sure it will work with such combination.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 23, 2024

AFAICT the code is here (below) .. whereby the this.removeOldChannel = true; in the constructor means that channels are deleted and recreated. Or??

https://github.com/openhab/openhab-core/blob/660102e3f93f928473b66f56f2955090dfa3c30e/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/update/UpdateChannelInstructionImpl.java#L106

@lolodomo
Copy link
Contributor

If @J-N-K could confirm or if someone could test it, that would be great and we could simply merge all the PRs.

@lsiepel
Copy link
Contributor

lsiepel commented Nov 23, 2024

I just tested this. My conclusion is that the itemType is updated with upgrade instructions just by updating the type attribute. I Also noticed that the contents / child attributes are updated even though the type id remains the same. E.g. you don't need to use a new type id just to get it updated.

This instruction:

		<instruction-set targetVersion="1">
			<update-channel id="valvePosition">
				<type>plugwiseha:valvePosition</type>
			</update-channel>
		</instruction-set>

Is enough to change:

 "uid": "plugwiseha:appliance_valve:a1e230fe97:3ba1e1c42b9d4a5faf74460bbb990981:valvePosition",
          "id": "valvePosition",
          "channelTypeUID": "plugwiseha:valvePosition",
          "itemType": "Number",
          "kind": "STATE",
          "label": "Valve Position",
          "description": "Gets the position of the valve (0% closed, 100% open)",
          "defaultTags": [
            "OpenLevel",
            "Status"
          ],
          "properties": {},
          "configuration": {}
        }

To:

 "uid": "plugwiseha:appliance_valve:a1e230fe97:3ba1e1c42b9d4a5faf74460bbb990981:valvePosition",
          "id": "valvePosition",
          "channelTypeUID": "plugwiseha:valvePosition",
          "itemType": "Number:Dimensionless",
          "kind": "STATE",
          "label": "Valve Position",
          "description": "Gets the position of the valve (0% closed, 100% open)",
          "defaultTags": [
            "OpenLevel",
            "Status"
          ],
          "properties": {},
          "configuration": {}
        }

@lolodomo if you are confident enough with these tests that the itemType is actually updated, i'll merge the color temperature PR's.

@andrewfg
Copy link
Contributor Author

andrewfg commented Nov 23, 2024

the itemType is updated with upgrade instructions just by updating the type attribute. I Also noticed that the contents / child attributes are updated even though the type id remains the same. E.g. you don't need to use a new type id just to get it updated.

I concur. That is what the OH core shows it is doing. The previous channel is totally destroyed, and a new channel is created from the channel-type; it does not care about the contents of the prior channel-type..

@lolodomo
Copy link
Contributor

@lsiepel : thank you a lot for testing that.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo lolodomo merged commit caf163e into openhab:main Nov 24, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.3 milestone Nov 24, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants