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

[Nanoleaf] Add channel for visualizing layout #13552

Merged
merged 13 commits into from
Nov 12, 2022

Conversation

austvik
Copy link
Contributor

@austvik austvik commented Oct 15, 2022

When adding a Nanoleaf device to your Openhab, each light panel will get a separate thing, identified by its ID. It is hard to figure out which ID is which panel, especially if you have many panels. The Nanoleaf binding has had a way to print the layout with the ids for the original type of Nanoleaf panels, but no good way to identify what is what for more modern panel types.

This pull request adds a Layout channel on the controller which returns an image with the panel ids, so that it is easier to find the IDs you are interested in.

This is only tested on Nanoleaf Elements Hexagons, but should work on many other shapes and types of panels.

Signed-off-by: Jørgen Austvik jaustvik@acm.org

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik austvik force-pushed the austvik/openhab-addons branch from 5ae3e79 to a06588c Compare October 15, 2022 12:48
@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Oct 15, 2022
@jlaur
Copy link
Contributor

jlaur commented Oct 18, 2022

@austvik - this is a very creative approach to solving that problem. :-) I would be curious to see how it looks.

However, I'm also thinking if creating such a channel for temporary use to identify IDs is the right approach. Perhaps you could have a look at #13555 which seems to solve a quite similar problem in a different way. Don't read this as I have anything against your approach, just consider it for inspiration. If I'm not mistaken and the problem is indeed similar, perhaps it would be worth settling on some common approach. In this case I would personally consider the console approach less bloated and more streamlined. WDYT?

Cc @lolodomo

@lsiepel
Copy link
Contributor

lsiepel commented Oct 18, 2022

@austvik - this is a very creative approach to solving that problem. :-) I would be curious to see how it looks.

However, I'm also thinking if creating such a channel for temporary use to identify IDs is the right approach. Perhaps you could have a look at #13555 which seems to solve a quite similar problem in a different way. Don't read this as I have anything against your approach, just consider it for inspiration. If I'm not mistaken and the problem is indeed similar, perhaps it would be worth settling on some common approach. In this case I would personally consider the console approach less bloated and more streamlined. WDYT?

Cc @lolodomo

If there is a need for a common approach isn't this visual solution based on a channel easier to use than a more techy console solution?

@jlaur
Copy link
Contributor

jlaur commented Oct 19, 2022

@austvik - could you provide a screenshot from this?

@lsiepel - I agree this approach is more visual and perhaps even more user-friendly. Maybe I was being a bit too harsh with my comment. :-) IMHO we can merge the PR as is. However, moving forward, it might make sense to harmonize the way of finding IDs which can otherwise be hard to find.

Please correct me here if I'm wrong:

  • User creates an item linked to the new channel.
  • IDs can now be seen from MainUI.
  • The IDs of interest are written down (copy'n'paste not possible).
  • The item is removed again to clean up.

With console:

  • Console is started.
  • Command is executed.
  • The IDs of interest can be copied.
  • Console is exited.

With the console approach you can even use SSH from remote and there is no runtime penalty. I haven't seen the visual approach yet, but I could imagine if more bindings would follow this road, there would be quite different visualizations with different layout, colors etc. Not saying it's a problem, just stating it probably won't be as neutral or "to the point" as a console command.

@austvik
Copy link
Contributor Author

austvik commented Oct 19, 2022

image
This is how my setup looks (as you can see, I am no designer :) - but it makes it possible to identify the elements.

I can change to a white background and have the lights and edges in different shades of gray if you want. Obvious possible enhancement is to read the colors from the things.

@austvik
Copy link
Contributor Author

austvik commented Oct 19, 2022

The layout implementation returns a byte[], so can easily be written to a file from the console. I just dont think it is user friendly. ssh in, find the users, find some directory the software has write access to, scp it back.

@lsiepel
Copy link
Contributor

lsiepel commented Oct 19, 2022

Would it be possible to have this as a bridge property instead of a channel?

@austvik
Copy link
Contributor Author

austvik commented Oct 19, 2022

No. Properties are strings in OpenHab.

public Map<String, String> getProperties();
public String setProperty(String string, String string1);
public void setProperties(Map<String, String> map);

@lsiepel
Copy link
Contributor

lsiepel commented Oct 19, 2022

IMHO would be the best place if you ask me. Not sure if it would ever be possible to also support images.

I see more usecases like solarpanel layout etc.

@austvik
Copy link
Contributor Author

austvik commented Oct 19, 2022

I totally agree. If properties were <String, State> instead of <String, String>, that is where it would fit.

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik
Copy link
Contributor Author

austvik commented Oct 19, 2022

image

Added some style to the image and only calculate the image if the channel is linked.

@stefan-hoehn
Copy link
Contributor

First of all, @austvik, this looks awesome. This is what I always wanted to do and I even started a branch on that many months ago but due to the blockly work I always pushed that back. So great YOU did it and much nicer than I did!

As you you probably know there IS already a console command available: see https://www.openhab.org/addons/bindings/nanoleaf/#panel-layout
but that only does it with characters and also it only supports squares.

I haven't yet understood how your channel idea works (I have not yet had time to look into the code) but my first implementation of that went by a channel as well and it was actually "rejected" by @kaikreuzer by then as channels are not supposed to be used for that. So give me a few days to look into it and to even try it out and will come back to you.

Also let me note that I got a documentation from nanoleaf how to "officially" calculate all shapes (it is actually a few handwritten pages full of math :-) ). I am happy to forward that to you.

So again, great that you did it. Just give me a few days (I am off next week and away from home with nanoleads), so expect something in around 2 weeks, okay? Btw, I have four different sets of panels and shapes to test everything here.

CU soon
Stefan

@austvik
Copy link
Contributor Author

austvik commented Oct 19, 2022

image
image

A couple of screenshots to show how the channel work. When you click the image you get native resolution.

I'm interrested in your document (jaustvik@acm.org), and would also love to get a copy of your JSON (panelLayout with globalOrientation and layout) to test the algorithm.

@stefan-hoehn
Copy link
Contributor

Expect a mail in your inbox just now (you may want to edit you comment above and remove the mail address if you like)

@kaikreuzer
Copy link
Member

my first implementation of that went by a channel as well and it was actually "rejected" by @kaikreuzer by then

This sounds too hard. 😉 We had a similar discussion with arguments @jlaur mentions above - usually the console is quite helpful for setting up things, so that was my advice at that time. Seeing @austvik's nice images and the fact that the hexagons are fairly difficult to render in ascii art, I'd also think that the channel on the bridge can be a suitable solution here (while indeed it is a bit of a misuse of channels). But creating an image on the file-system through the console is indeed no option as it would be hard to transfer it to a place where the user can actually look at it.

The item is removed again to clean up.

@jlaur I'd expect in practise that users will simply leave the item in place. They can at any time look at the layout and also have it updated whenever it changes. The implementation should merely make sure to not send item updates too often as this will mean quite some traffic on the event bus...

So from my pov: Let's go ahead with that solution, I like it! 👍

@jlaur
Copy link
Contributor

jlaur commented Oct 20, 2022

The item is removed again to clean up.

@jlaur I'd expect in practise that users will simply leave the item in place. They can at any time look at the layout and also have it updated whenever it changes. The implementation should merely make sure to not send item updates too often as this will mean quite some traffic on the event bus...

In the current implementation it will be updated each minute (with the default configured refresh interval) if the channel is linked to any items (i.e. when not cleaning up after use). @austvik - it might be worth considering if this could be triggered on actual changes. I have not checked if this is feasible, but I'll await your reply before merging.

So from my pov: Let's go ahead with that solution, I like it! 👍

Great, so we have consensus to accept and merge this. :-) After seeing the actual rendered result, it seems more complex that I had imagined, so I agree that the graphical visualization certainly adds value over the console approach for this case.

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik
Copy link
Contributor Author

austvik commented Oct 22, 2022

I've added changes so that it only renders the layout if the layout has changes (and is linked to an item), and it now supports triangles, hexagons and squares.

image
image
image
image

@stefan-hoehn
Copy link
Contributor

stefan-hoehn commented Oct 31, 2022

Thanks for checking my layouts as well. I love that. I love what it creates! I also love the fact that it only happens when the layout changes and that you have added the panel id's to it. Btw, it seems my panels don't seem to be hanging completely aligned on the wall (it can tell by looking on the non-straight lines of the drawings that are produced) ;-)

Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

First of all, don't be frightened or even annoyed about the many comments, rather take it as an appreciation of all the great effort you have done to the Nanoleaf binding. This is something I always wanted to do and you finally delivered it and probably much better than I would have done!
Thanks for all your hard work,
Stefan

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik
Copy link
Contributor Author

austvik commented Nov 1, 2022

The thing that is not fixed is the null checking in the equals. I think it is correct, but it is written a little but strange to make the compiler understand that some @nullable variable can't be null. If you think it is OK, then we can go with it, if not I can put the "both null => return" case first?

@stefan-hoehn
Copy link
Contributor

I actually trust you there as I've reviewed the full code you provided pretty thoroughly which to me looks overall really good. Whatever way you feel is good, let's go for that.

@austvik
Copy link
Contributor Author

austvik commented Nov 2, 2022

OK, then I think it would be good to get this in. Ill see if I can do something for the colors this weekend. I might be able to generalize the corners then. I think getting colors for shapes is much easier than for elements corners.

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik
Copy link
Contributor Author

austvik commented Nov 3, 2022

I have tried to rewrite the hard equals methods. From my POV I think this is ready now? - but I am not familiar with the process.

@austvik austvik requested a review from stefan-hoehn November 3, 2022 21:04
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 your contribution! I have added only some very minor comments.

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik
Copy link
Contributor Author

austvik commented Nov 11, 2022

Thank you, Jacob. I believe your comments are now fixed.

@jlaur
Copy link
Contributor

jlaur commented Nov 11, 2022

@austvik - something went wrong with last commit: A new file HexagonCorners was added, containing duplicate class Hexagon. Also, the changes in nanoleaf_de.properties must be completely reverted.

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik
Copy link
Contributor Author

austvik commented Nov 12, 2022

Thanks @jlaur! I've removed the extra file and the unintentional changes to the German translation.

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 - thanks!

@jlaur jlaur merged commit fbd06ec into openhab:main Nov 12, 2022
@jlaur jlaur added this to the 3.4 milestone Nov 12, 2022
@jlaur
Copy link
Contributor

jlaur commented Nov 12, 2022

@austvik - thanks again. Perhaps PR title could be rephrased slightly, for example: "Add channel for visualizing layout"? That would give users reading release notes a direct hint how to utilize this without further reading.

theater pushed a commit to theater/openhab-addons that referenced this pull request Nov 13, 2022
* Visualize Nanoleaf layout
* Only calculate image if channel is linked
* White background image
* Render more shapes

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@austvik austvik changed the title [Nanoleaf] Visualize layout [Nanoleaf] Add channel for visualizing layout Nov 14, 2022
@austvik
Copy link
Contributor Author

austvik commented Nov 14, 2022

Thanks everybody!

And good suggestion on the PR title - I have updated it.

@stefan-hoehn
Copy link
Contributor

A final "thanks" for the contribution - I love it.

andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 22, 2022
* Visualize Nanoleaf layout
* Only calculate image if channel is linked
* White background image
* Render more shapes

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
@stefan-hoehn
Copy link
Contributor

andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Dec 24, 2022
* Visualize Nanoleaf layout
* Only calculate image if channel is linked
* White background image
* Render more shapes

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
* Visualize Nanoleaf layout
* Only calculate image if channel is linked
* White background image
* Render more shapes

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* Visualize Nanoleaf layout
* Only calculate image if channel is linked
* White background image
* Render more shapes

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* Visualize Nanoleaf layout
* Only calculate image if channel is linked
* White background image
* Render more shapes

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Jan 6, 2024
* Visualize Nanoleaf layout
* Only calculate image if channel is linked
* White background image
* Render more shapes

Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
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