107-Arduino-Cyphal icon indicating copy to clipboard operation
107-Arduino-Cyphal copied to clipboard

Refactor arduino wrapping system (2)

Open aentinger opened this issue 2 years ago • 5 comments

Supersedes #136, rebased on 107-Arduino-UAVCAN:master and PR against 107-Arduino-UAVCAN:master.

aentinger avatar Sep 30 '21 07:09 aentinger

Hi @moiflo033 :wave: :coffee: I've found another :bug: :wink:

  • When generating the typedefs around service request/response messages CanardTransferKindMessage is wrongly used instead of CanardTransferKindRequest and CanardTransferKindResponse. Can you please fix this?
-typedef DSDLBaseType<uavcan_node_ExecuteCommand_Request_1_0, uavcan_node_ExecuteCommand_Request_1_0_SERIALIZATION_BUFFER_SIZE_BYTES_, CanardTransferKindMessage> Request_1_0;
+typedef DSDLBaseType<uavcan_node_ExecuteCommand_Request_1_0, uavcan_node_ExecuteCommand_Request_1_0_SERIALIZATION_BUFFER_SIZE_BYTES_, CanardTransferKindRequest> Request_1_0;
-typedef DSDLBaseType<uavcan_node_ExecuteCommand_Response_1_0, uavcan_node_ExecuteCommand_Response_1_0_SERIALIZATION_BUFFER_SIZE_BYTES_, CanardTransferKindMessage> Response_1_0;
+typedef DSDLBaseType<uavcan_node_ExecuteCommand_Response_1_0, uavcan_node_ExecuteCommand_Response_1_0_SERIALIZATION_BUFFER_SIZE_BYTES_, CanardTransferKindResponse> Response_1_0;
  • Also it would be highly desirable that the namespace has the version number, i.e. namespace ExecuteCommand_1_0 { instead of ... Response_1_0.

aentinger avatar Sep 30 '21 07:09 aentinger

Another wish for @moiflo033 :pray: Please invite me as a collaborator to your fork so I can directly push on your branch. That's not possible anymore, now that I opened a new pull request myself :pray:

aentinger avatar Sep 30 '21 07:09 aentinger

Invite sent.

moiflo033 avatar Sep 30 '21 07:09 moiflo033

Codecov Report

Merging #137 (9d72167) into master (532d3a2) will decrease coverage by 0.66%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   85.51%   84.84%   -0.67%     
==========================================
  Files          17       11       -6     
  Lines         642      363     -279     
==========================================
- Hits          549      308     -241     
+ Misses         93       55      -38     
Impacted Files Coverage Δ
src/ArduinoUAVCAN.ipp 100.00% <100.00%> (+4.54%) :arrow_up:
src/wrappers/DSDLBaseType.hpp 100.00% <100.00%> (ø)
src/wrappers/DSDLBaseType.ipp 100.00% <100.00%> (ø)
src/ArduinoUAVCAN.cpp 82.75% <0.00%> (-13.80%) :arrow_down:
src/nunavut/support/serialization.h 91.04% <0.00%> (-0.85%) :arrow_down:
src/types/uavcan/node/ExecuteCommand_1_0.h
src/wrappers/uavcan/node/Version_1_0.hpp
src/wrappers/uavcan/node/ExecuteCommand_1_0.hpp
src/types/uavcan/node/Version_1_0.h
src/utility/convert.hpp
... and 3 more

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 532d3a2...9d72167. Read the comment docs.

codecov-commenter avatar Sep 30 '21 07:09 codecov-commenter

https://github.com/107-systems/107-Arduino-UAVCAN/pull/136#pullrequestreview-767384836:

I recall that this is already somewhat handled by nunavut, maybe @pavel-kirienko can advise on a quick remedy for this situation?

Use the id filter: https://nunavut.readthedocs.io/en/latest/docs/templates.html#c-filters

Example: https://github.com/UAVCAN/nunavut/blob/a76bfeda30fb15640def41fd940ada552b77ce5d/src/nunavut/lang/c/templates/definitions.j2#L153

Directly inserting identifiers from DSDL into the generated code is unsafe.

pavel-kirienko avatar Sep 30 '21 12:09 pavel-kirienko