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

Where does nunavut generated files get copied to ? #72

Closed
echoGee opened this issue Jan 28, 2021 · 12 comments · Fixed by #78
Closed

Where does nunavut generated files get copied to ? #72

echoGee opened this issue Jan 28, 2021 · 12 comments · Fixed by #78
Labels
type: support Request for help using the project.

Comments

@echoGee
Copy link
Contributor

echoGee commented Jan 28, 2021

There is a src folder where the
-types
--uavcan
-utility
....
is present I see that the automatically generated header files are all edited to account for the fact that Arduino cannot be given an additional -I flag to include the "types" folder. This would mean that all the autogenerated files are manually edited.
Is this right ?

@echoGee echoGee changed the title Where does nunavut generated files be copied to ? Where does nunavut generated files get copied to ? Jan 28, 2021
@aentinger
Copy link
Member

Hi 👋 ☕
This is by and large correct. The nunavut-generated header files are slightly changed to (string/search/replace) be findable by the arduino build system, e.g.

< #include <uavcan/node/Health_1_0.h>
< #include <uavcan/node/Mode_1_0.h>
---
> #include "Health_1_0.h"
> #include "Mode_1_0.h"

However, a short lookup unearthed arduino/arduino-cli#502 - which - if it is merged should allow you to specify additional include paths.

@aentinger aentinger added the type: support Request for help using the project. label Jan 29, 2021
@echoGee
Copy link
Contributor Author

echoGee commented Jan 29, 2021

I think this is a bit too far to support the out of box functionality. The same solution could be done with

  • Copying nunavut files to sketch folder, without any modification. The wrappers would have to change if there is a significant change, but otherwise should work
  • Include the nunavut folder with the -I option. But this is not on the Arduino GUI ? Is that accurate ?

@aentinger
Copy link
Member

Include the nunavut folder with the -I option. But this is not on the Arduino GUI ? Is that accurate ?

Yes, not yet. It might be in the future.

However, I do believe that it makes sense to store both generated C headers and handcoded wrappers both in the same repository so that the user does not have to work with the nunavut generated headers at all (as it currently is with all types provided by 107-Arduino-UAVCAN). If you think about it: Why would you store the wrappers that refer to and depend an the generated header files but not store the header files themselves? And if the DSDL changes it's as simple as creating a new library version of 107-Arduino-UAVCAN and distribute it via the library manager system.

(Storing all generated C header files + Arduino wrappers is currently discussed here: #65)

@echoGee
Copy link
Contributor Author

echoGee commented Feb 1, 2021

If the DSDL's are edited, there is a manual process to copy and paste them. Keeping the wrapper and the DSDLs separate creates a lot more clarity in the developing environment IMHO. There isn't a need to copy store another script to rename the header files or copy the nunavut files to another location if they are separate. Imagine if there are few tens to 100s of DSDL files that need to be copied.

Arduino, from what I understand, is primarily a good stepping stone for someone starting to use a new architecture/framework. However, if that stepping stone is paving the way to two different implementation approaches, it may result in hobbyist, companies following these practices moving forward including shipping beta products with them(I know I have done that. ) ? It is a step that may be retracted, but is there a value from providing conflicting implementation habits ?

@echoGee
Copy link
Contributor Author

echoGee commented Feb 2, 2021

@aentinger I understand that the suggestion mentioned is a bit different from the previous convention, but could we adopt the new convention?

@aentinger
Copy link
Member

Hi 👋 I'm still not quite sure what you are advocating for - maybe you can clarify? As I wrote above too, I don't see any sense in checking in wrappers while simultaneously not checking in the header files those wrappers are referring too. I agree with the opinion voiced by Scott over here. As a consequence I'd provide at least the already defined and released uavcan data types within this library.

As reg/drone will only be extended (but not changed anymore) those types could be provided in a similar way. Certainly beats the approach that every user has to rewrite his wrappers from scratch. As an Arduino library I'm obliged to make thinks simple and not, as Scott put it, take software best practice approaches into account.

@echoGee
Copy link
Contributor Author

echoGee commented Feb 3, 2021

I am not against storing the header files. Here are the suggestions in short:

  • Store the header files and wrappers and track them
  • Change the header files and wrappers to different folders. This is expected in a real development environment since nunavut expects headers to be output to a folder. In my opinion, it is a bit more clear to keep the wrappers and header files in separate folder as there wouldn't be too many similar looking file names in the same folder. If I remember right, Arduino does similar practices(not for the same reason, but) to split header files in libraries to an include directory and cpp files to src directory.

I think this adds zero complexity to a new user while providing the right structure to take the next step.

@aentinger
Copy link
Member

Thank you for clarifying your message, I do believe I understand now what you are talking about and I feel our opinions are quite (although not completely) overlapping already. Here's what I propose (also see this branch):

  • Store generated headers (.h) and wrappers (.hpp) in the same directory (and under src/types) as shown below. I really do not want to keep duplicate folder structures where one contains the wrappers and the other the headers. I also think little of keeping them in all in the top-level source directory as this would seriously mess-up the organisation of the source code.
107-Arduino-UAVCAN/src/types$ tree reg/
reg/
└── drone
    ├── physics
    │   └── electricity
    │       ├── Power_0_1.h
    │       ├── PowerTs_0_1.h
    │       ├── Source_0_1.h
    │       ├── SourceTs_0_1.h
    │       └── SourceTs_0_1.hpp
    └── service
        ├── battery
        │   ├── Error_0_1.h
        │   ├── Parameters_0_1.h
        │   ├── Parameters_0_1.hpp
        │   ├── Status_0_1.h
        │   ├── Status_0_1.hpp
        │   └── Technology_0_1.h
        └── common
            ├── Heartbeat_0_1.h
            └── Readiness_0_1.h
  • Provide name clashes of equivalent named wrapper types by encapsulating them in namespaces, e.g.
namespace reg { 
namespace drone {
namespace service {
namespace battery {

template <CanardPortID ID>
class Parameters_0_1
  • Enter all the include paths within ArduinoUAVCANTypes.h. This file in turn is included in ArduinoUAVCAN.h which is the top-level-include of this Arduino library. Consequently you've got all types directly available without having to manually including another header file.
#include "types/reg/drone/physics/electricity/SourceTs_0_1.hpp"

#include "types/reg/drone/service/battery/Parameters_0_1.hpp"
#include "types/reg/drone/service/battery/Status_0_1.hpp"

However, what I will not be able to do is to support the 0.2 version of battery message types which currently only exists in your fork. I will limit support and inclusion into 107-Arduino-UAVCAN only to those types pushed upstream into https://github.com/UAVCAN/public_regulated_data_types.

@echoGee
Copy link
Contributor Author

echoGee commented Feb 5, 2021

  • Store generated headers (.h) and wrappers (.hpp) in the same directory (and under src/types) as shown below. I really do not want to keep duplicate folder structures where one contains the wrappers and the other the headers. I also think little of keeping them in all in the top-level source directory as this would seriously mess-up the organisation of the source code.

What is the downside of keeping two duplicate folder structures.

types/reg/
          └── drone
              ├── physics
              │   └── electricity
              │       ├── Power_0_1.h
              │       ├── PowerTs_0_1.h
              │       ├── Source_0_1.h
              │       ├── SourceTs_0_1.h
              └── service
                  ├── battery
                  │   ├── Error_0_1.h
                  │   ├── Parameters_0_1.h
                  │   ├── Status_0_1.h
                  │   └── Technology_0_1.h
                  └── common
                      ├── Heartbeat_0_1.h
                      └── Readiness_0_1.h

and another

wrappers/reg/
            └── drone
                ├── physics
                │   └── electricity
                │       └── SourceTs_0_1.hpp
                └── service
                    ├── battery
                    │   ├── Parameters_0_1.hpp
                    │   ├── Status_0_1.hpp
                    └── common

This helps with

  • clarity on which files are autogenerated files,
  • clarity on which wrappers are currently implemented
  • Ease of copying and updating the nunavut generated header files when there are updates, or when users have custom generated ones.

Isn't creating a duplicate folder structure (not files) providing so much more clarity than the extra space overhead it creates ?

However, what I will not be able to do is to support the 0.2 version of battery message types which currently only exists in your fork. I will limit support and inclusion into 107-Arduino-UAVCAN only to those types pushed upstream into https://github.com/UAVCAN/public_regulated_data_types.

This has made it to master branch with a PR

@aentinger
Copy link
Member

I'll think over your suggestion re folder structure. As your PR made it to master there's nothing standing in the way of supporting that too. We could then also include your BMS example sketch here.

@aentinger
Copy link
Member

I've mediated on your desire for a duplicate folder structure and came to the conclusion that the advantages outweigh the benefits. Now you could go ahead and add support for version 0.2 of the battery messages.

@echoGee
Copy link
Contributor Author

echoGee commented Feb 5, 2021

very cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: support Request for help using the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants