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

Include SetUUID for SSDP #4981

Merged
merged 3 commits into from
Jul 30, 2018
Merged

Include SetUUID for SSDP #4981

merged 3 commits into from
Jul 30, 2018

Conversation

laercionit
Copy link
Contributor

Inclusion of the SetUUID Method for Custom UUID

Inclusion of the SetUUID Method for Custom UUID
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Overall this looks ok to me, except for y minor comments.

"<modelURL>%s</modelURL>"
"<manufacturer>%s</manufacturer>"
"<manufacturerURL>%s</manufacturerURL>"
"<UDN>UUID: %s</UDN>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The text uuid changed to uppercase and a space was added after the colon. Is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific purpose, just aesthetic.
It gets sharper for technical support information.

@@ -39,7 +39,7 @@ class UdpContext;
#define SSDP_SCHEMA_URL_SIZE 64
#define SSDP_DEVICE_TYPE_SIZE 64
#define SSDP_FRIENDLY_NAME_SIZE 64
#define SSDP_SERIAL_NUMBER_SIZE 32
#define SSDP_SERIAL_NUMBER_SIZE 37
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change on purpose? If so, could you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I increased to support larger serial numbers.
When I was testing, I felt this need.

Depending on the manufacturer, the serial number format may be somewhat similar to UUID in size.

void setDeviceType(const String& deviceType) { setDeviceType(deviceType.c_str()); }
void setDeviceType(const char *deviceType);
void setName(const String& name) { setName(name.c_str()); }
void setUUID(const String& uuid) { setUUID(uuid.c_str()); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment that this must be called before begin() for user-specified uuid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, make comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"<manufacturer>%s</manufacturer>"
"<manufacturerURL>%s</manufacturerURL>"
"<UDN>uuid:%s</UDN>"
"</device>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100% sure, but I believe these were left here in case a user wants to add a specific icon. It's not the best way to do it, but until a better way is implemented I'd rather keep these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% right either. As the code was commented, I found it appropriate to remove, but no problem to maintain. You decide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally I'd agree, or I'd even request the removal myself, but I think this should be an exception. Please keep it for now, I consider this technical debt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@devyte devyte merged commit 75a6a3f into esp8266:master Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants