Extend model classes with small int types, node type and string-based node query #110
Conversation
BjoernAtBosch
changed the title
| /** | ||
| * @brief A tree node. | ||
| * | ||
| */ | ||
| class Node { | ||
| public: | ||
| enum class Type { | ||
| BRANCH, | ||
| UNKNOWN_LEAF_TYPE, |
There was a problem hiding this comment.
There exists in COVESA VSS also nodes for structs and properties. Any particular reason to limit this one to leaf types, or could we have it more general as UNKNOWN_TYPE? (And if so have it first?)
There was a problem hiding this comment.
Currently, we do not support structs and properties.
A general UNKNOWN_TYPE could be discussed.
There was a problem hiding this comment.
Having spent some time to think about this: I don't like defining an unknown for enums so much, as it always raises the question how to handle this value.
I introduced the UNKNOWN_LEAF_TYPE just for backwards compatibility reasons, means in case the SDK is used by a model generated before doing these extensions.
I've a question regarding your hint regarding structs and properties. At least for structs my understanding is that this isn't a node type but a data type: A struct represents a datapoint/signal of complex nature and can be either of type attribute, sensor, or actuator. I.e. those node types shall not be mixed within a single struct type.
There was a problem hiding this comment.
Yes, the struct/property VSS nodes will never appear in the same tree as signals and branches.
Thanks for the explanation. Possibly add a comment on UNKNOWN_LEAF_TYPE that it is there just for backward compatibility reasons
| @@ -48,15 +60,24 @@ class Node { | |||
| */ | |||
| [[nodiscard]] std::string getPath() const; | |||
|
|
|||
| [[nodiscard]] virtual Type getType() const { return Type::BRANCH; } | |||
There was a problem hiding this comment.
Why is branch always returned?
I assume it is because we only instantiate a Node for Branch, in all other case we have a subclass of Node that overrides this with something else, or? Maybe worth a comment?
There was a problem hiding this comment.
Yes, a "pure" Node always represents a branch. I'll add a comment ...
| } | ||
|
|
||
| int16_t GrpcDataPointValueProvider::getInt16Value() const { | ||
| return static_cast<int16_t>(getDataPoint().int32_value()); |
There was a problem hiding this comment.
Are we assuming that Databroker has checked that the value will fit into an int16_t?
( I believe it in most cases is a fair assumption)
Do we have, or would it be easy to add unit tests for the new methods?
There was a problem hiding this comment.
Don't know if it's worth to have unit tests for this, but I could add an assert statement
Describe your changes
These changes are required for extending the performance app to allow measuring actuation performance of the Velocitas SDK. Extension with node type and small int types are also later required for aligning the vehicle model expected by the app at build time with the model provided by the data broker at runtime.
Issue ticket number and link
Checklist - Manual tasks
Examples are executing successfully
Created/updated unit tests. Code Coverage percentage on new code shall be >= 80%.
Created/updated integration tests.
Devcontainer can be opened successfully
Devcontainer can be opened successfully behind a corporate proxy
Devcontainer can be re-built successfully
Extended the documentation (e.g. README.md, CONTRIBUTING.md, Velocitas Docs)