search

PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.27k stars 13.42k forks source link

UAVCAN v1 Node Issue Tracker and Discussion #16948

Open mrpollo opened 3 years ago

mrpollo commented 3 years ago

Working towards getting UAVCAN v1 support following the DS-015 standard.

Team Working on This feature

Done

TODO

Hardware Supported

Manufacturer Type Hardware Build Name Bootloader Apps Status Contributors
NXP Peripheral FMUK66 nxp_fmuk66_v3_socketcan ✅ ✅ GPS with UCANS32K146, BMS with BMS-772 @PetervdPerk-NXP
Holybro Peripheral Pixhawk 4 TODO ✅ ✅ TODO @JacobCrabill
mRo Peripheral Pixracer TODO ✅ ✅ TODO @JacobCrabill
Zubax Peripheral Babel TODO ✅ ✅ TODO @JacobCrabill

Pull Requests

Add Publishers and Subscribers for UAVCAN v1 (Merged) https://github.com/PX4/PX4-Autopilot/pull/16889

Actuator Output Control https://github.com/PX4/PX4-Autopilot/pull/16808

coderkalyan commented 3 years ago

I'm going to try to implement the GNNS topic fix and run this on the CUAV CAN GPS as discussed in the call. Might take me some time as I don't know how this works yet :smile:

pavel-kirienko commented 3 years ago

this is kinda a side question, but is there any chance Nunavut could create an enum for the type of a register Value object? I know there's a _tag_ variable, but then it just compares that to some "magic numbers" as far as the code using the header file is concerned. Obviously this all comes from the DSDL file, but I'd prefer to avoid circumventing direct output of the DSDL by hard-coding an enum in my code.

@JacobCrabill we discussed it at some point but IIRC we found no major usage for an enum like you are describing. If you have a specific use case in mind, then consider submitting a patch to Nunavut, please, or at least open an issue. Thanks in advance.

There is a much safer and cleaner approach based on metaprogramming but implementing that would require rewriting the parameter binder from scratch, not sure if anyone is interested. Otherwise, I can describe it here.

I would be very interested; I scratched my head for a while trying to think of how this should be done then got impatient and just implemented the simplest version I could to keep the momentum going. Not sure if it would be something to include in this PR, but it could at least be a follow-up effort.

The general idea is to define an interface for your parameter binder and make a class template parameterized by the register type that implements the interface: template <typename RegisterType> class RegisterBinder : public IRegisterBinder { ... };

Define a pair of methods for reading and writing the register. Each method would compare its RegisterType against each field of Value_1_0 and choose one accordingly, then copy the value from/to the PX4 parameter.

The type comparison can be either hand-coded or if you can use C++17 you can improve safety and readability by listing all value types in std::variant and traversing them at compile time using std::visit. I can provide an example here if needed.

The alternative is to use lambda as I suggested. Either way, the current implementation is non-compliant and will cause compatibility issues.

You may want to be aware of the C++ code generation target @thirtytwobits is working on: https://github.com/UAVCAN/nunavut/compare/main...thirtytwobits:issue/91. If you could lend him a hand here, we could replace the C-generated code with C++ to make your implementation cleaner and more typesafe while still using libcanard, irrespective of the above parameter thing.

JacobCrabill commented 3 years ago

Adding a link to @PetervdPerk-NXP 's latest PR (already merged) for reference here: https://github.com/PX4/PX4-Autopilot/pull/17376

JacobCrabill commented 3 years ago

A still-open issue we need to address in the future is type-casting within the UAVCAN parameter system: https://github.com/PX4/PX4-Autopilot/pull/16889#discussion_r582032230

pavel-kirienko commented 3 years ago

I would like to add a cautionary note here regarding Peter's patch to libcanard that reduces the memory consumption at the expense of introducing a linear-time lookup procedure into the RX pipeline:

image

While it does save you 508 bytes of RAM per subscription, it makes the temporal properties of the overall system variable and dependent on the network configuration. The original design implemented in libcanard guarantees that if your application works on your table in a network consisting of 2 nodes, it will also function identically in a system consisting of X nodes. This is a crucial guarantee for a real-time system. The patch removes it, making the system susceptible to failure depending on the network configuration.

I am not necessarily saying that the patch is inadmissible, but the subtle complications that it introduces may not be worth the 508 bytes of RAM it saves.

PetervdPerk-NXP commented 3 years ago

Hi Pavel, as of now I don't have plans to upstream that code at all and I'm aware of the negative impact on determinism of the system.

However my development board is running out of RAM and for me this was quick way to keep on going since it saved ~ 10KB RAM. And thus I can just continue on development of PX4 code that's more important.