Fix topic and type name handling

[github-actions[bot]]

Static memory analysis

Default configuration

MTU: 512 B Input buffer size: 2048 B Input history: 4 Output buffer size: 2048 B Output history: 4

Entity	Qty	Size per unit
Context	2	5616 B
Topic	8	56 B
Service	4	248 B
Client	4	248 B
Subscription	4	280 B
Publisher	4	288 B
Node	4	208 B
Static input buffer	8	2136 B
Init options	6	64 B
Wait sets	4	56 B
Guard Condition	4	64 B

TOTAL: 34500 B

[Acuadros95]

LGTM

[codecov-commenter]

Codecov Report

Merging #254 (53df2d3) into main (77c4b22) will decrease coverage by 1.06%. The diff coverage is 25.58%.

@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
- Coverage   49.44%   48.37%   -1.07%     
==========================================
  Files          44       44              
  Lines        1711     1728      +17     
  Branches      484      498      +14     
==========================================
- Hits          846      836      -10     
- Misses        642      661      +19     
- Partials      223      231       +8     

Impacted Files	Coverage Δ
rmw_microxrcedds_c/src/rmw_client.c	45.37% <0.00%> (-4.18%)	:arrow_down:
rmw_microxrcedds_c/src/rmw_microxrcedds_topic.c	43.54% <0.00%> (-10.16%)	:arrow_down:
rmw_microxrcedds_c/src/rmw_service.c	45.00% <0.00%> (-4.13%)	:arrow_down:
rmw_microxrcedds_c/src/utils.c	27.60% <64.70%> (-1.92%)	:arrow_down:

---

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 77c4b22...53df2d3. Read the comment docs.

[pablogs9]

Thanks, @Acuadros95, let's wait for @gavanderhoorn feedback before merging.

[gavanderhoorn]

I haven't tested this yet, but I noticed the functions like generate_topic_name(..) now only return a bool.

snprintf(..) can fail in a nr of different ways, so it seems like returning a more fine-grained status/error code from generate_topic_name(..) et al. would make reporting the different failure modes from calling code also easier/possible.

Other places in Micro-ROS are very careful in how they handle and report errors (such as the other parts of the ctor of rmw_client_t, a few lines up), so it would align with that approach.

What do you think?

[pablogs9]

snprintf will report that chars that should have been written are more than the buffer array, in that case, a false is returned.

And the function will fail here for example https://github.com/micro-ROS/rmw_microxrcedds/pull/254/files#diff-fa1104ae182dc7153d689fedbab8020d187abaafeb7e9a6829346f040060302aR82

[gavanderhoorn]

I was just wondering whether we'd want to differentiate between 'buffer overrun' and 'other error', which according to snprintf(3) is possible.

[pablogs9]

Could you propose a user input that can end as "other error" in the internal usage of snprintf(3)?

[gavanderhoorn]

No, I can't come up with such a situation, but that's not really how I normally judge whether or not to handle return values the documentation tells me are possible :)

As I wrote, I was just curious whether you felt it would be beneficial to be able to distinguish between the different failure modes. More specific error messages have helped me in the past while debugging Micro-ROS issues (mostly in my personal use of the API, but still).

I'm OK with the proposed changes.

[pablogs9]

@mergify backport galactic foxy humble

[mergify[bot]]

backport galactic foxy humble

✅ Backports have been created

* [#255 Fix topic and type name handling (backport #254)](https://github.com/micro-ROS/rmw_microxrcedds/pull/255) has been created for branch `galactic` * [#256 Fix topic and type name handling (backport #254)](https://github.com/micro-ROS/rmw_microxrcedds/pull/256) has been created for branch `foxy` * [#257 Fix topic and type name handling (backport #254)](https://github.com/micro-ROS/rmw_microxrcedds/pull/257) has been created for branch `humble`

[gavanderhoorn]

Thanks for the quick turnaround btw. 👍