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

Domain Model #1554

Merged
merged 17 commits into from
Nov 25, 2022
Merged

Domain Model #1554

merged 17 commits into from
Nov 25, 2022

Conversation

cstoidner
Copy link
Contributor

@cstoidner cstoidner commented Nov 2, 2022

Proposed changes

Documentation of the thin-edge domain-model.

That PR is a replacement for discontinued PR #1195.

Last unresolved domain-model comments from discontinued PR #1195 are listed below as TODOs. All those TODOs shall be considered/resolved in that PR; for each an individual comment shall be opened.

  1. Synopsis: "This syntax is used in several places in this document, but sounds really weird."
    Link: child-device requirements #1195 (comment)

  2. Synopsis: "simplify this by removing "sensors, actuators or domain applications."
    Link: child-device requirements #1195 (comment)

  3. Synopsis: "Why do we use 'external' here? (external child-device)"
    Link: child-device requirements #1195 (comment)

  4. Synopsis: "I am a bit irretated by introducing "metric" here."
    Link: child-device requirements #1195 (comment)

  5. Synopsis: "This device model is focused on OT devices, while thin-edge will run elsewhere. I find this confusing."
    Link: child-device requirements #1195 (comment)

  6. Synopsis: "The source of a metric it not necessarily a device - it can be a process."
    Link: child-device requirements #1195 (comment)

  7. Synopsis: "Having "Command" under "telemetry" heading feels a bit strange to me."
    Link: child-device requirements #1195 (comment)

  8. Synopsis: "we should explain somewhere in more details our approach (device-twin) here,
    e.g. that commands do a device state change and then update the device twin afterwards]"
    Link: child-device requirements #1195 (comment)

  9. Synopsis: "this section (data concept) is not domain-model related."
    Link: child-device requirements #1195 (comment)

  10. Synopsis: "better term for plugin"
    Link: child-device requirements #1195 (comment)

  11. Synopsis: "It would be good to explain that DM is conceptually based on the doamin model terms above."
    Link: child-device requirements #1195 (comment)

  12. Synopsis: "I think we are missing a section for the "Inventory data" which is neither measurement nor metrics."
    Link: child-device requirements #1195 (comment)

  13. Synopsis: Improvement for definition of 'device twin'
    Link: child-device requirements #1195 (comment)

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#1260

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@cstoidner cstoidner self-assigned this Nov 2, 2022
@cstoidner cstoidner mentioned this pull request Nov 2, 2022
11 tasks
@cstoidner
Copy link
Contributor Author

cstoidner commented Nov 2, 2022

TODO (5)

Synopsis: "This device model is focused on OT devices, while thin-edge will run elsewhere. I find this confusing."
Link: #1195 (comment)

With the latest structure changes focused on OT devices is gone.

[...] connecting to other devices which may or may not have thin-edge running but need device managment e.g. Sony use case

Also that "gateway case" is covered now.

@cstoidner
Copy link
Contributor Author

TODO (1)

Synopsis: "This syntax is used in several places in this document, but sounds really weird."
Link: #1195 (comment)

Entire document was changed according to the suggestion (see https://github.com/cstoidner/thin-edge.io/commit/0a9449d1f76ff94e2131937fe4584cff218b3fc4).

@cstoidner
Copy link
Contributor Author

TODO (2)

Synopsis: "simplify this by removing "sensors, actuators or domain applications."
Link: #1195 (comment)

Suggestion applied, see https://github.com/cstoidner/thin-edge.io/commit/cfe5163bc6a396d6643412ee6dff39050fb67955

@cstoidner
Copy link
Contributor Author

TODO (3)

Synopsis: "Why do we use 'external' here? (external child-device)"
Link: #1195 (comment)

The term "external child-device" is used, since beside those also processes or containers on the main-device can be handled as "logical child-devices". I added now an according hint, see https://github.com/cstoidner/thin-edge.io/commit/da26172f73e0f8109f3c53e09bc2d406ca550463

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Can you please:

Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
@cstoidner
Copy link
Contributor Author

Can you please:

* clean the commit history inherited from #1195

* sign you commit with `commit -s`

* read and follow the CONTRIBUTING.md guide

Done.

Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
@cstoidner
Copy link
Contributor Author

TODO (6)

Synopsis: "The source of a metric it not necessarily a device - it can be a process."
Link: #1195 (comment)

[...] collectd is a source of measurement that is not represented as a child device.

I see. I adopted the definition of metric now, see https://github.com/cstoidner/thin-edge.io/commit/e5e47a334d2db1aa939cbd02c2fcb9ddf5d8087f

More details about that shall be later considered in the data model.

@cstoidner
Copy link
Contributor Author

cstoidner commented Nov 4, 2022

TODO (7)

Synopsis: "Having "Command" under "telemetry" heading feels a bit strange to me."
Link: #1195 (comment)

New response in old PR:

"I see the term command is really missleading here.
[...]
Instead I prefer to name it setpoint."

See #1195 (comment)

Replaced now command with setpoint, see 719912e

Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
@cstoidner
Copy link
Contributor Author

TODO(12)

Synopsis: "I think we are missing a section for the "Inventory data" which is neither measurement nor metrics."
Link: #1195 (comment)

I added the inventory, see e9c186a

@reubenmiller , please have a look.

@cstoidner
Copy link
Contributor Author

TODO (13)

Synopsis: Improvement for definition of 'device twin'
Link: #1195 (comment)

Also insprired by your suggestion I decoupled the concept 'telemtry data' from state now.
See https://github.com/cstoidner/thin-edge.io/commit/e4c57206351183fc906717cae3ba2a3fea0dcb93

Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
@cstoidner
Copy link
Contributor Author

TODO (4)

Synopsis: "I am a bit irretated by introducing "metric" here."
Link: #1195 (comment)

[...] The more I investigate about metrics and measurements I came now also to the clue, that metric is not the right thing here. [...]
[...] So I am about to remove "metrics" from the model and adapt the definition of measurements. [...]

I removed now the concept "metric", but added "sample" underneath the measurements. See c6bbf0c

@cstoidner
Copy link
Contributor Author

cstoidner commented Nov 8, 2022

TODO (11)

Synopsis: "It would be good to explain that DM is conceptually based on the doamin model terms above."
Link: #1195 (comment)

[...] This include software, configuration and log management. The management uses above terms (commands, measurements) to interact with the IoT Cloud.

Software, configuration and log management do not really base on commands or measurements. So I see nothing to add.

I assume your comment is a result of that missleading term of commands. Fortunately that was removed now (see #1554 (comment) above). Sorry for that.

@cstoidner
Copy link
Contributor Author

TODO (9)

Synopsis: "this section (data concept) is not domain-model related."
Link: #1195 (comment)

Document was restructured and that entire section "thin-edge data concept" was removed now.

Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
@cstoidner
Copy link
Contributor Author

@cstoidner cstoidner closed this Nov 9, 2022
@cstoidner cstoidner reopened this Nov 9, 2022
@cstoidner
Copy link
Contributor Author

cstoidner commented Nov 9, 2022

TODO (10)

Synopsis: "better term for plugin"
Link: #1195 (comment)

New suggestion in old PR:

"[...] what do you think about the term add-on, using for components like c8y-config-plugin, or similar 3rd party or customer specific components?
[...]"

See #1195 (comment)

Still to be aligned about a suitable term.

@cstoidner
Copy link
Contributor Author

TODO (8)

Synopsis: "we should explain somewhere in more details our approach (device-twin) here,
e.g. that commands do a device state change and then update the device twin afterwards]"
Link: #1195 (comment)

[...]
That's why it must be introduced, and even better explained.

Proposal:
thin-edge has been designed with the assumption [...]

Suggested explanation for "device twin" is adopted now.

[...] we should explain somewhere in more details our approach [...]

More details can be added into a specific document.

Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
docs/src/architecture/domain-model.md Show resolved Hide resolved
docs/src/architecture/domain-model.md Show resolved Hide resolved
docs/src/architecture/domain-model.md Outdated Show resolved Hide resolved
docs/src/architecture/domain-model.md Outdated Show resolved Hide resolved
docs/src/architecture/domain-model.md Show resolved Hide resolved
docs/src/architecture/domain-model.md Outdated Show resolved Hide resolved
docs/src/architecture/domain-model.md Outdated Show resolved Hide resolved
docs/src/architecture/domain-model.md Outdated Show resolved Hide resolved
docs/src/architecture/domain-model.md Outdated Show resolved Hide resolved
docs/src/architecture/domain-model.md Show resolved Hide resolved
cstoidner and others added 7 commits November 16, 2022 16:38
Co-authored-by: Albin Suresh <albinsuresh@hotmail.com>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Signed-off-by: Christoph Stoidner <c.stoidner@gmx.de>
Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

I'm okay to have this merged - as long as the commits are squashed.

However, I find this text really too dry with a long list of definitions with little context and definitions. Hope this will be improved in the future.

Comment on lines +156 to +165
## Inventory

**thin-edge** holds and manages an **inventory** on the **main-device**, that stores and provides information about the **main-device** and known **child-devices**
* information stored per **device** are
* supported kinds of **Device Management** capabilities/operations
* supported kinds of **Telemetry Data**
* optionally any custom-specific meta-information per **device**
* the **inventory** is the communication backbone for **plugins**, **external child-devices**, the **domain application**[^1] and **thin-edge** it-self
* one can add information to announce capabilities a **device** supports
* another one can retrieve those information to identify capabililties a **device** supports
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing of that section has been currently implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats true. Same for the new telemetry item setpoint and telemetry details units and samples; and finally new device management functionality firmware management.

For all those development work need to be planned.

@cstoidner cstoidner merged commit b063c3c into thin-edge:main Nov 25, 2022
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

4 participants