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

Initial DIAL (http://www.dial-multiscreen.org/) client. #2136

Merged
merged 28 commits into from
Nov 7, 2020

Conversation

slaff
Copy link
Contributor

@slaff slaff commented Nov 3, 2020

Allows Sming applications to control remote monitors/TVs.
@piperpilot For example a Sming application can open YouTube on a smart TV and display a video when the BBQ is ready.

@slaff slaff added this to the 4.2.0 milestone Nov 3, 2020
@slaff slaff changed the title Initial DIAL (http://www.dial-multiscreen.org/) client. [WIP] Initial DIAL (http://www.dial-multiscreen.org/) client. Nov 4, 2020
@slaff slaff changed the title [WIP] Initial DIAL (http://www.dial-multiscreen.org/) client. Initial DIAL (http://www.dial-multiscreen.org/) client. Nov 5, 2020
@slaff slaff force-pushed the feature/multimedia-dial-client branch from 00a6c21 to a1e8446 Compare November 5, 2020 09:33
@slaff slaff requested a review from mikee47 November 5, 2020 09:36
@mikee47 mikee47 force-pushed the feature/multimedia-dial-client branch from 84c1141 to cbb5515 Compare November 5, 2020 23:21
Copy link
Contributor

@mikee47 mikee47 left a comment

Choose a reason for hiding this comment

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

I've made a few suggestions via commits (take 'em or leave 'em). On namespaces should we use DIAL for consistency?

@slaff
Copy link
Contributor Author

slaff commented Nov 6, 2020

I've made a few suggestions via commits (take 'em or leave 'em).

Thanks. All looks good to me.

On namespaces should we use DIAL for consistency?

Better not. Better have the same coding style for namespaces as for classes. Which reminds me that mDns should be renamed to Mdns and it is probably a good idea to do it before the 4.2 release.

@mikee47
Copy link
Contributor

mikee47 commented Nov 6, 2020

On namespaces should we use DIAL for consistency?

Better not. Better have the same coding style for namespaces as for classes. Which reminds me that mDns should be renamed to Mdns and it is probably a good idea to do it before the 4.2 release.

I think the main reason for upper-camelcase names is consistency to avoid silly typos, although that should be negated using eclipse, etc. It feels more important that the name is correct, which for network protocols means TCP, mDNS, UPnP, SSDP, etc. There's a good reason you picked mDNS in the first place!

May I suggest we simply add namespace aliases, i.e. namespace Mdns = mDns and could even add MDNS and mdns? I can do the same with SSDP/UPnP. That would allow application developers to use whichever convention feels natural to them, but libraries would always use the correct one internally.

@mikee47
Copy link
Contributor

mikee47 commented Nov 6, 2020

@slaff I'm doing some revisions to UPnP library which should help to streamline this PR. Should have them out today.

@@ -7,7 +7,9 @@
#define WIFI_PWD "PleaseEnterPass"
#endif

static Dial::Client client;
namespace
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikee47 I will remove the anonymous namespace. If we apply this change it should be applied to all samples for consistency. Which can be part of a separate PR.

@mikee47
Copy link
Contributor

mikee47 commented Nov 6, 2020

@slaff The Dial::Client is very interesting as it demonstrates behaviour that UPnP::ControlPoint should handle. Specifically, tracking unique service names and fetching descriptions. I could pull that out now, or leave it for another PR?

@slaff
Copy link
Contributor Author

slaff commented Nov 6, 2020

I could pull that out now,

@mikee47 Do you mean to create a separate repository for the Dial client? If yes, I will make this. I wanted to change also the naming of one flash variable... let me do it first...

@slaff
Copy link
Contributor Author

slaff commented Nov 6, 2020

Specifically, tracking unique service names and fetching descriptions. I could pull that out now, or leave it for another PR?

Or you have meant to put the tracking and fetching of the description from Dial::Client into UPnP::ControlPoint ? If yes - let's finish with this PR and you can make your changes in a separate PR for it. Is that ok for you?

@mikee47
Copy link
Contributor

mikee47 commented Nov 6, 2020

@slaff I've kind of done it already :-) I'll push the changes and see what you think - also found an important bug in TCP.

@mikee47 mikee47 force-pushed the feature/multimedia-dial-client branch from 27e6592 to a96d038 Compare November 6, 2020 18:04
@mikee47
Copy link
Contributor

mikee47 commented Nov 6, 2020

@slaff Thank you for your work in getting this control point stuff started, looking really good! I've opened an issue to discuss and track changes necessary before Sming 4.2 release.

@slaff
Copy link
Contributor Author

slaff commented Nov 6, 2020

@mikee47 Shall I wait for you or better merge it as it is in develop and the move it into a separate repository?

@mikee47
Copy link
Contributor

mikee47 commented Nov 6, 2020

@slaff I think go ahead and move it, feels like a good place to shove a stick in the mud.

@slaff slaff removed the 3 - Review label Nov 7, 2020
@slaff slaff merged commit a76c061 into SmingHub:develop Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants