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

Creating a BACnetAndModbusController and a IoTHubConnectionParameter #234

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

cbupp
Copy link
Collaborator

@cbupp cbupp commented Mar 13, 2023

I have a device that is a Gateway, a BACnetController and a ModbusController.

DTDLv2 limits the number of models a new model can extend to 2. To extend Gateway, BACnetController, and ModbusController, I'm proposing a BACnetAndModbusController that extends both BACnetController and ModbusController. I've tested this with Azure Digital Twins and confirmed that this inheritance is accepted. Not sure if this impacts SHACL.

I also need to represent the IoT Hub Connection information in the ADT. I believe it makes the most sense to extend the Parameter model. I've used the REC DTMI, though, maybe this should go through the BrickSchema? Or maybe it's reconciled at a later time. I've created a GatewayConnectionParameter as well to try and future proof this.

@hammar - I couldn't find documentation on the file naming decisions. For the BACnetAndModbusController file name, I went with BACnetAndModbusController.json. The directories seem to have a mix of abbreviated names and full names.

@cbupp cbupp requested a review from hammar March 13, 2023 15:23
@hammar
Copy link
Contributor

hammar commented Mar 30, 2023

Let's go over this at TC next week. I have two reflections:

  1. Our tooling for docs generation isn't designed to support multiple inheritance, so I am a little unsure as to how that will work when this PR is merged (the docs are regenerated by the CI automation). This may require a bit of work.
  2. Agree that a Parameter for IoT Hub connection details makes sense. But also I'm wary of the consequences of adding REC->Brick->REC inheritance, and again, what the tools (current and future) will do with that. May require a bit of thinking in case/when we go to a DTDL->SHACL translation pipeline.

Conceptually and content-wise, I don't see anything wrong with this content. And we shouldn't let prior versions of tools that we built block innovation here, so don't take the above as blockers.

@cbupp
Copy link
Collaborator Author

cbupp commented Mar 30, 2023

@hammar - I did confirm that at least A document is generated when multiple inheritance occurs:
image

Regarding SHACL: I hadn't considered the impact to SHACL.

@cbupp
Copy link
Collaborator Author

cbupp commented Apr 5, 2023

2. But also I'm wary of the consequences of adding REC->Brick->REC inheritance

Doesn't the ICTEquipment also follow a REC->Brick->REC inheritance?

@hammar
Copy link
Contributor

hammar commented Apr 5, 2023

  1. But also I'm wary of the consequences of adding REC->Brick->REC inheritance

Doesn't the ICTEquipment also follow a REC->Brick->REC inheritance?

Yes, and the same applies there :)

@cbupp
Copy link
Collaborator Author

cbupp commented Apr 5, 2023

Comments from REC TC 4/3/23:

  • Update PR with an ODR
  • Review supporting tooling to ensure inheritance model is workable

@cbupp
Copy link
Collaborator Author

cbupp commented Apr 18, 2023

@hammar - thinking this through further with my team, we've decided that a lot of industry "Controllers" support multiple protocols (BACnet, Modbus, others) and abstract out the protocol itself through firmware functionality.

We no longer think it makes sense to extend BACnetController or ModbusController, and that these advanced controllers should instead extend just the "Controller" model.

I'm thinking of removing the BACnetAndModbusController models, but keeping the rest. I think the 2 ODRs also make sense to keep, I'll just tweak the context for the multiple inheritance ODR.

@hammar
Copy link
Contributor

hammar commented Apr 26, 2023

Have at long last looked through assumptions in DTDL2OAS and DTDL2MD, which are part of the GitHub workflows to regenerate API spec and documentation, respectively. I've found no reason to assume they'll break by either multi-parent classes, or the inheritance hierarchy jumping REC -> Brick -> REC.

Feature-wise, while DTDL2MD does not break per se, it does not show all routes to the top in the breadcrumb trail, only the longest path. Propose we update this prior to next release, for the quality of the docs to be up to par. Note that the index page will list the interface as child node of multiple parents, as expected.

The DTDL2SHACL tool could probably have an issue with the latter REC->Brick->REC jumping, which would require a bit of engineering work when generating SHACL models prior to any release (either to update said tool, or manually adjust the generated SHACL). But as we are working primarily with the DTDL models on a daily dev cycle, that is not something that needs to block any DTDL work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to remove colon?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to remove colon?

@cbupp
Copy link
Collaborator Author

cbupp commented Nov 9, 2023

I've updated this PR to reflect the REC Technical Committee conversations:

  1. Modern controllers support multiple protocols. There is no need to represent the protocol as a sub type of controller. That would result in very messy merging to represent changeable firmware on a controller.
  2. I've updated the GatewayConnectionParameter ODR to use 002 as the documented decision index.

@cbupp cbupp merged commit bd50063 into main Nov 9, 2023
1 check passed
@cbupp
Copy link
Collaborator Author

cbupp commented Nov 9, 2023

Approved for merge at REC TC 11/9/23

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.

2 participants