Draft the design document and prepare a rough mockup of the C++ API #1
Conversation
docs/daemon.hpp
Outdated
| virtual std::expected<std::unique_ptr<RPCClient>, Error> make_client(const dsdl::Type& type, | ||
| const std::uint16_t service_id) = 0; |
There was a problem hiding this comment.
Does the Client need to include the server_node_id as a parameter?
There was a problem hiding this comment.
Yes, forgot this one. I think we'll go the same way we did in LibCyphal and PyCyphal -- the server node-ID is to be the factory parameter.
docs/DESIGN.md
Outdated
|
|
||
| - Yakut is a developer tool, while OCVSMD is a well-packaged component intended for deployment in production systems. | ||
|
|
||
| - Yakut is a user-interactive tool with a CLI, while OCVSMD is equipped with a machine-friendly interface -- a C++ API. Eventually, OCVSDM may be equipped with a CLI as well, but it will always come secondary to the well-formalized C++ API. |
There was a problem hiding this comment.
OCVSDM may be equipped with a CLI as well
Any concerns with including daemon in the name if it's also going to be a CLI? I know we're still deciding on the name but might be something to consider.
There was a problem hiding this comment.
The CLI would be a different entry-point/executable (I think)
There was a problem hiding this comment.
The CLI sits on top of the daemon, so I think it is fine.
docs/DESIGN.md
Outdated
|
|
||
| Dynamic DSDL loading is proposed to be implemented by creating serializer objects whose behavior is defined by the DSDL definition ingested at runtime. The serialization method is to accept a byte stream and to produce a DSDL object model providing named field accessors, similar to what one would find in a JSON serialization library; the deserialization method is the inverse of that. Naturally, said model will heavily utilize PMR for storage. An API mockup is given in `dsdl.hpp`. | ||
|
|
||
| One approach assumes that instances of `dsdl::Object` are not exchanged between the client and the daemon; instead, only their serialized representations are transferred between the processes; thus, the entire DSDL support machinery exists in the client's process only. This approach involves certain work duplication between clients, and may impair their ability to start up quickly if DSDL parsing needs to be done. Another approach is to use shared-memory-friendly containers like Boost Interprocess or specialized PMR. |
There was a problem hiding this comment.
I think there are two approaches to define:
- public-regulated types
- all other types
For 1. we should simply create the types in C++ and make them part of the client library (we could use Nunavut with custom templates to generate these at build-time) with serialization being opaque to the client.
The first implementation we could cheat this by exchanging the objects directly over unix domain sockets and then work in type-safe message exchange later.
For 2. the dsdl::Object approach seems better suited.
|
I added edits per what we discussed above and earlier this morning. Please give them another look and comment if there is anything to add. I will keep this MR open for now because it doesn't inhibit our progress yet. |
docs/DESIGN.md
Outdated
|
|
||
| - Report the progress via `uavcan.node.Heartbeat.1.vendor_specific_status_code`. This is rejected because the VSSC is vendor-specific, so it shouldn't be relied on by the standard. | ||
|
|
||
| The plan that is tentatively agreed upon is to define a new standard message with a fixed port-ID for needs of progress reporting. The message will likely be placed in the diagnostics namespace as `uavcan.diagnostic.ProgressReport` with a fixed port-ID of 8183. The `uavcan.node.ExecuteCommand.1` RPC may return a flag indicating if the progress of the freshly launched process will be reported via the new message. |
There was a problem hiding this comment.
With fixed port-id approach, for me it looks like there can be potentially only one operation in progress. IMO more flexible approach would be if ExecuteCommand has a new field with desired port id (f.e. with progressPortId field name) where server should post uavcan.diagnostic.ProgressReport-s for this operation we are executing. If this progressPortId is set (>0) then it means that client wants progresses. With this approach...:
- we don't need "RPC may return a flag indicating if the progress ..." thing
- RTP client can decide whether progresses are needed or not
- potentially multiple simultaneous commands could be executed, each with their own progress msgs flow
Alternately, especially if a fixed port-ID is desired, we should at least add some unique operation id (int64?) into ExecuteCommand and into corresponding ProgressReport messages, so that it will be possible to distinguish to which operation this particular progress msg is related to. Again, we can use default 0 operation id as indication that client doesn't want progresses for this command.
There was a problem hiding this comment.
With this approach...:
I was originally reluctant to agree to this because opening a new publisher on the updatee is a highly fallible operation that is better done at the initialization stage, but then I concluded that the advantages probably outweigh the disadvantages. You have already enumerated the advantages of this approach correctly. I might add that the fixed port-ID solution is too reminiscent of DroneCAN in its rigidity, and if one wants DroneCAN, one knows where to find it.
In the most basic case, each node sending ExecuteCommand requests would have a subscriber for the progress reports configured via a register; the same subject-ID will be sent to the clients when progress reports are desired.
In this case we don't need a new message type as we can just leverage uavcan.primitive.scalar.Natural8.1.
There was a problem hiding this comment.
I believe how we currently handle this in our software install is that we query a register on the updatee. Would that be sufficient?
yakut register [NODE_ID] updater.download.status
There was a problem hiding this comment.
@lydia-at-amazon That would work, but: we considered a similar solution where we add a progress field to the response of uavcan.protocol.ExecuteCommand. Scott pointed out that an RPC-based solution is considered harmful, so we focused on pub/sub based alternatives. If RPC is acceptable, then I think your current register-based approach is probably the most obvious choice.
There was a problem hiding this comment.
For maintenance operations, such as software update, we're already using lots of RPC services, so I think it should be ok to use the RPC-based solution.
The most important part at the moment is the firmware update. You will notice that my proposal differs from what is suggested in the work order. I would like to discuss this in practical terms.
The rest seems mostly straightforward; I have attempted to capture the main idea of that modem daemon API in my design.