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

Cleanup type headers in preparation for supporting all types. #65

Merged
merged 4 commits into from
Jan 24, 2021

Conversation

aentinger
Copy link
Member

@aentinger aentinger commented Jan 11, 2021

No description provided.

…r file in preparation for support all uavcan types.
…rentiating between nunavut generated C headers (.h) and Arduino UAVCAN type headers (.hpp)
…avut generated C headers. Instead we are using .h to identify those auto-generated headers and .hpp to identify Arduino wrappers.
@aentinger aentinger added topic: firmware Code that runs on an embedded system. priority: high type: enhancement PR to improve the project. labels Jan 11, 2021
@aentinger aentinger self-assigned this Jan 11, 2021
@codecov-io
Copy link

codecov-io commented Jan 11, 2021

Codecov Report

Merging #65 (da38bc7) into master (33392d6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   85.37%   85.37%           
=======================================
  Files          18       17    -1     
  Lines         506      506           
=======================================
  Hits          432      432           
  Misses         74       74           
Impacted Files Coverage Δ
src/types/uavcan/node/Health_1_0.h 84.84% <ø> (ø)
src/types/uavcan/node/Heartbeat.1.0.hpp 100.00% <ø> (ø)
src/types/uavcan/node/Heartbeat_1_0.h 71.42% <ø> (ø)
src/types/uavcan/node/ID.1.0.hpp 100.00% <ø> (ø)
src/types/uavcan/node/ID_1_0.h 75.75% <ø> (ø)
src/types/uavcan/node/Mode_1_0.h 84.84% <ø> (ø)
src/types/uavcan/node/Version.1.0.hpp 100.00% <ø> (ø)
src/types/uavcan/node/Version_1_0.h 80.48% <ø> (ø)
src/types/uavcan/node/ExecuteCommand.1.0.hpp 100.00% <100.00%> (ø)
src/types/uavcan/node/ExecuteCommand_1_0.h 78.04% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3ed044...da38bc7. Read the comment docs.

// You shouldn't attempt to edit this file.
//
// Checking this file under version control is not recommended unless it is used as part of a high-SIL
// safety-critical codebase. The typical usage scenario is to generate it as part of the build process.
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is a sensible move?

A question one is likely to ask here is: Why don't you ship precompiled regulated DSDL together with the tool? Indeed, that would be trivial to do, but we avoid that on purpose to emphasize our commitment to supporting vendor-specific and regulated DSDL at the same level. In the past we used to give regulated namespaces special treatment, which caused our users to acquire misconceptions about the purpose of DSDL. Specifically, there have been forks of the standard namespace extended with vendor-specific types, which is harmful to the ecosystem.

Surely there should be a way to use vendor-specific types. If that is the case, then probably the same principles could be applied to the standard data types as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure this is a sensible move?

No? I get the feeling that I'm misunderstanding the UAVCAN concept a little. So the regulated types in uavcan are not supposed to be used individually and should only be used by incorporating them into a DSDL composite type representing some sensor/actuator with the workflow being: 1) create DSDL for your sensor/actuator using uavcan regulated types, 2) generate C headers for your sensor/actuator and 3) use those C headers in your application? If so I'm a bit at loss how to "Arduino-fy" this workflow.

Copy link
Member

Choose a reason for hiding this comment

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

Not quite, no. It is possible to use the standard types by themselves, but I would recommend ensuring that vendor- or application-specific types are as easy to use as the standard ones, otherwise it may foster misconceptions as I wrote in the excerpt above. I think a better approach would be to somehow enable Arduino users to easily generate headers for arbitrary namespaces, whether it's uavcan, reg, or whatever. Shall that turn out to be impossible, I am willing to consider having a trivial website wrapping Nunavut that lets you upload a DSDL namespace (or a link to GitHub repository containing one) and spits generated code back.

I would like to tag @thirtytwobits because he might have relevant ideas.

Copy link
Member Author

Choose a reason for hiding this comment

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

Providing a web-service around the DSDL generator might be a sensible move. You've correctly picked up on my concern that requiring the typical Arduino user to self-generate the C header files might be one step too much. Also it's contradictory with allowing the usage of the library out of the box without any further setup steps.

Furthermore there's the issue of some "convenience" typedefs and functions which I fail to see how to code-generate them. Therefore my idea was to periodically generate the C headers and merge changes into this repository, preserving the convenience functionality and Arduino wrappers.

But really, I'm open for anything that might lead to a self-contained and easy-to-use Arduino library (wrapper of UAVCAN).

Copy link
Member

Choose a reason for hiding this comment

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

Furthermore there's the issue of some "convenience" typedefs and functions which I fail to see how to code-generate them.

Excellent examples. The first one is to be eventually supported by DSDL language itself, but for now, we have to limit ourselves to regular constants. The other one is an interesting shortcut that some might describe as an abuse of operator overloading, but at any rate, it is trivial to automate using Nunavut template language. DSDL in its current form is a pretty spartan language that has some potential for improvement.

Regarding the web service -- can we, perchance, recruit some resources from your team? From our side, we can dispatch our glorious sysadmin Clyde to get the web service functional and deployed but he will need help with the business logic.

Choose a reason for hiding this comment

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

Starting work on C++ generation here -> https://github.com/thirtytwobits/nunavut/tree/issue/91

Choose a reason for hiding this comment

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

The Arduino-way to do this sort of thing is to be very device-specific and have a set of DSDL for a given device as a separate library that utilizes a core library (see Adafruit's core GFX library and the individual libraries for various displays they sell as an example). As such, I'd provide the pre-generated headers for the core standard in a central UAVCAN/Arduino library and create libraries for each UAVCAN device supported that had pre-generated headers for that device and examples that utilized them.

Most Arduino projects are started with the example sketches that are distributed with a library so these device-specific libraries tend to be the entry-points that quietly pull-in the core libraries they extend.

This scheme also lets you push update notifications to the IDE which is better then having people generate code on their own since any bugs we find in the serialization logic can be fixed and the users notified by the library update mechanism.

In-short, I do think distributed pre-generated headers is appropriate for Arduino. It is a specific ecosystem where software development best practices don't always apply.

Copy link
Member

Choose a reason for hiding this comment

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

How would an Arduno user transpile custom DSDL in this case?

Choose a reason for hiding this comment

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

Using your website I suppose but the most important thing about Arduino is the immediacy. You need to have a way to run a fully-functional example before you can expect users to move on to advance concepts like IDLs and transpilers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Scott, thank you very much for providing your perspective on this. I'm agreeing with you that pre-generated headers are the way to go for Arduino (which is what I initially set out to do within this PR). Adopters of this library already have set out to create their own wrappers so instead of having a rank growth of every adopter writing some wrappers and header to provide their types, why not providing them all from the source library (this one).

In short: I'm still pro storing C generated headers and manually (tiresome, but doable) Arduino wrappers within this library.

@aentinger aentinger added the status: blocked Progress on this is prevented by an external cause. label Jan 20, 2021
@aentinger aentinger changed the title Add support for all uavcan types. Transfer C generated headers + Arduino wrappers directly to examples. Jan 20, 2021
@aentinger aentinger changed the title Transfer C generated headers + Arduino wrappers directly to examples. Cleanup type headers in preparation for supporting all types. Jan 20, 2021
@aentinger aentinger marked this pull request as ready for review January 24, 2021 14:18
@aentinger aentinger merged commit 85c1b77 into master Jan 24, 2021
@aentinger aentinger deleted the all-uavcan branch January 24, 2021 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high status: blocked Progress on this is prevented by an external cause. topic: firmware Code that runs on an embedded system. type: enhancement PR to improve the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants