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

[epsonprojector] Add SDDP discovery #16802

Merged
merged 8 commits into from
Jun 5, 2024
Merged

Conversation

mlobstein
Copy link
Contributor

@mlobstein mlobstein commented May 25, 2024

This PR adds SDDP discovery for epson projectors using the core SDDP discovery service.

Depends on openhab/openhab-core#4237

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein mlobstein 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 awaiting other PR Depends on another PR labels May 25, 2024
@mlobstein mlobstein requested a review from andrewfg May 25, 2024 16:19
@mlobstein
Copy link
Contributor Author

@lolodomo once this is finished, it should be easy to copy-paste into sonyprojector.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein mlobstein removed the work in progress A PR that is not yet ready to be merged label May 29, 2024
@holgerfriedrich holgerfriedrich added rebuild Triggers Jenkins PR build awaiting other PR Depends on another PR and removed rebuild Triggers Jenkins PR build awaiting other PR Depends on another PR labels May 30, 2024
@lolodomo
Copy link
Contributor

lolodomo commented May 31, 2024

@lolodomo once this is finished, it should be easy to copy-paste into sonyprojector.

Sony is also using its own protocol named SDAP for discovery. But SDDP is also supported. I need host, port and model to create a thing. I see these fields are provided by object SddpDevice. Do you know why port field is a String ? I see you are not using it, any reason ?

For the model, I cannot be sure that they will provide the exact same string I am using. One option would be to let the user define the model. I can also try to set it in case I can identify an approximative matching.

Unfortunately, I will not be able to test that because my model has no Ethernet connection.

@jlaur
Copy link
Contributor

jlaur commented May 31, 2024

@mlobstein - do you know if there are any projectors supporting only AMX, but not SDDP? I'm wondering if it would still make sense to keep AMX support in case some older projectors doesn't support other discovery methods?

@andrewfg
Copy link
Contributor

Do you know why port field is a String ?

In SddpDevice we extract the port from one of the headers being the part after the colon in a the header which contains ipAddress:port. Normally this should indeed be an integer. However since the respective header comes from the remote device, it could possibly contain a non numeric value. And rather than throw a number conversion exception in core, we pass that as a string to the binding developer so they can decide themselves. Often the port returned is 1902 which is simply the port of the SDDP multicast group, so often not much use when sending unicast commands to a device. We basically provide it "just in case"..

@mlobstein
Copy link
Contributor Author

@mlobstein - do you know if there are any projectors supporting only AMX, but not SDDP? I'm wondering if it would still make sense to keep AMX support in case some older projectors doesn't support other discovery methods?

My ancient-came-out-in-2009 projector that I use for testing supports only AMX. The newer ones seem to support both. I was worried about getting duplicate discovery results for those in the supports both column and that was my deciding factor for removing AMX.

@mlobstein
Copy link
Contributor Author

@lolodomo once this is finished, it should be easy to copy-paste into sonyprojector.

Sony is also using its own protocol named SDAP for discovery. But SDDP is also supported. I need host, port and model to create a thing. I see these fields are provided by object SddpDevice. Do you know why port field is a String ? I see you are not using it, any reason ?

For the model, I cannot be sure that they will provide the exact same string I am using. One option would be to let the user define the model. I can also try to set it in case I can identify an approximative matching.

Unfortunately, I will not be able to test that because my model has no Ethernet connection.

For Sony and the ethernetconnection thing, I would think you would just use the default port of 53484 for PJ Talk. The model is trickier... Ideally the model specified by the SDDP packet could be matched up with one of the available models specified by the thing definition and if there was no match, fall back to the AUTO option. But I admit I don't know much about the sonyprojector implementation to say if that is an acceptable outcome.

@lolodomo
Copy link
Contributor

lolodomo commented Jun 1, 2024

For Sony, yes I will use default port 53484 and AUTO as model.

@jlaur
Copy link
Contributor

jlaur commented Jun 1, 2024

I was worried about getting duplicate discovery results for those in the supports both column and that was my deciding factor for removing AMX.

It's possible to have overlapping discovery (i.e. multiple methods) as long as they are kept consistent (see #14919). In particular they need the same representation property.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@mlobstein mlobstein changed the title [epsonprojector] Replace AMX discovery with SDDP [epsonprojector] Add SDDP discovery Jun 1, 2024
@mlobstein
Copy link
Contributor Author

I was worried about getting duplicate discovery results for those in the supports both column and that was my deciding factor for removing AMX.

It's possible to have overlapping discovery (i.e. multiple methods) as long as they are kept consistent (see #14919). In particular they need the same representation property.

Ok, I put AMX discovery back as it was.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@andrewfg
Copy link
Contributor

andrewfg commented Jun 1, 2024

Remove dashes from mac to prevent duplicate discovery results with AMX

Just for the avoidance of doubt .. are you making sure to use lower case in both representation properties?

@mlobstein
Copy link
Contributor Author

mlobstein commented Jun 1, 2024

Remove dashes from mac to prevent duplicate discovery results with AMX

Just for the avoidance of doubt .. are you making sure to use lower case in both representation properties?

That is a good point.. In the AMX packet it is uppercase. So I will put toUpperCase() on the SDDP side so that the representation property of both match..

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@lolodomo
Copy link
Contributor

lolodomo commented Jun 4, 2024

@jlaur @andrewfg : is it ok for you now ?

@jlaur
Copy link
Contributor

jlaur commented Jun 4, 2024

is it ok for you now ?

Yes, my only concern was if discovery support for older devices would be dropped, but since the AMX discovery is left intact, this is fully addressed.

@andrewfg
Copy link
Contributor

andrewfg commented Jun 4, 2024

is it ok for you now ?

Yes, I think so.

@lsiepel
Copy link
Contributor

lsiepel commented Jun 4, 2024

Can be merged when #16802 (comment) is handled.

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, thank you

@lolodomo lolodomo merged commit 43fa2c7 into openhab:main Jun 5, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.2 milestone Jun 5, 2024
@mlobstein mlobstein deleted the epson-sddp branch June 5, 2024 13:36
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Jun 16, 2024
* Replace AMX discovery with SDDP

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
psmedley added a commit to psmedley/openhab-addons that referenced this pull request Jun 16, 2024
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
* Replace AMX discovery with SDDP

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* Replace AMX discovery with SDDP

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
* Replace AMX discovery with SDDP

Signed-off-by: Michael Lobstein <michael.lobstein@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