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

Adding discovery schema only (not sequencer) #268

Merged
merged 17 commits into from
Mar 18, 2022

Conversation

grafnu
Copy link
Collaborator

@grafnu grafnu commented Mar 16, 2022

No description provided.

docs/specs/discovery.md Outdated Show resolved Hide resolved
// an implicit object point enumeration.
//
// Sent on MQTT topic: /devices/{device_id}/events/discovery
// where {device_id} is that of the discovery node (e.g. gateway). The {device_id}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The last sentence is confusing because it uses the same {device_id} - I assume it's to say that if a discovered device has a IoT ID, then this is provided in the families.iot.id field but the message is always published to the discovery node (/devices/{discovery node id}/events/discovery)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes -- it's tricky because it's some nuanced information that isn't going to be explained in a single comment. I've tried to make it a bit better, but I think here it just needs to be technically accurate, but it's not trying to explain everything.

"additionalProperties": false,
"patternProperties": {
"^[a-z][a-z0-9]*(_[a-z0-9]+)*$": {
"^iot|bacnet|ipv4|ipv6|ethmac$": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we explicitly define these as properties? e.g. in https://gist.github.com/noursaidi/f8431051eed682de0fd967cd8745f68b ?
Allows for validation and avoids ambiguity, e.g. the BACnet ID you gave was hexadecimal, generally it's number but I've seen it as a hexadecimal in the documentation for an older system. Similarly to be pedantic about Mac addresses, I've seen both upper and lower case, delimited by either : or - .

Even though it's easy to still interpret the results, where I think it's important is building protocols and gateways. A gateway configuration which uses a hexadecimal ID will stop working if the physical gateway is replaced with a gateway which uses a numeric ID, unless we specify _a BACnet gateway should support both a numeric and hexadecimal ID, but then it's just easier to pick one and stick to it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want to be as specific/pedantic as possible about the various formats, otherwise it's crazyland. E.g. a bacnet address is really just a 24-bit number, and we should stipulate that it's always an UPPER-CASE HEX Address. That said, I'm not sure the schema is the right place to enforce all of that (rather than in code somewhere), since it can get really ugly really quickly. So, "explicitly define them as properties" -- sure, but I'd still want them to be homogenous properties, not different (too hard to maintain).

It wasn't clear from your comment exactly what you were proposing... (define as properties vs. heterogeneous definitions).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible for one device to participate in multiple address families?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but that is supported by the "patternProperties" block. What this says is that "all properties in this block must match this given pattern" -- so, as long as the pattern can match different key names, multiple properties can exist. In this case, e.g., { iot, bacnet } would both match, so it would allow having two properties "iot" and "bacnet"...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, that's great.

I think the schema is a better place to express any formatting constraints because it will require less digging to see the format, and it can't drift if code and docs are out of sync.

docs/specs/sequences/discovery.md Outdated Show resolved Hide resolved
docs/specs/sequences/discovery.md Outdated Show resolved Hide resolved
docs/specs/sequences/discovery.md Outdated Show resolved Hide resolved
bin/loop_sequences Show resolved Hide resolved
docs/specs/discovery.md Outdated Show resolved Hide resolved
"additionalProperties": false,
"patternProperties": {
"^[a-z][a-z0-9]*(_[a-z0-9]+)*$": {
"^iot|bacnet|ipv4|ipv6|ethmac$": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible for one device to participate in multiple address families?

schema/common.json Outdated Show resolved Hide resolved
"additionalProperties": false,
"patternProperties": {
"^[a-z][a-z0-9]*(_[a-z0-9]+)*$": {
"^iot|bacnet|ipv4|ipv6|ethmac$": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, that's great.

I think the schema is a better place to express any formatting constraints because it will require less digging to see the format, and it can't drift if code and docs are out of sync.

@grafnu grafnu merged commit a4600cc into faucetsdn:master Mar 18, 2022
@grafnu grafnu deleted the schemaonly branch March 18, 2022 16:37
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.

None yet

3 participants