rclc_parameter: Fix rcl return values #270

bjsowa · 2022-03-18T16:23:03Z

I was looking through the code and noticed a potential problem with the way return values are processed in rclc_parameter functions.
RCL_RET_OK is defined as 0, so doing bitwise and operations on it will not change its value.
I think the author meant to use bitwise or operators instead. That way if one of the function results in an error, the return value will not be equal to RCL_RET_OK.

pablogs9 · 2022-03-21T06:46:22Z

Good catch

pablogs9 · 2022-03-21T06:46:33Z

@mergify backport foxy galactic

mergify · 2022-03-21T06:46:47Z

backport foxy galactic

🟠 Waiting for conditions to match

merged [:pushpin: backport requirement]

Hey, I reacted but my real name is @Mergifyio

pablogs9

LGTM

codecov-commenter · 2022-03-21T07:03:16Z

Codecov Report

Merging #270 (c813edc) into master (449575c) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   64.67%   64.58%   -0.10%     
==========================================
  Files          16       16              
  Lines        2197     2197              
  Branches      647      647              
==========================================
- Hits         1421     1419       -2     
- Misses        460      461       +1     
- Partials      316      317       +1

Impacted Files	Coverage Δ
...lc_parameter/src/rclc_parameter/parameter_server.c	`73.20% <100.00%> (ø)`
rclc/src/rclc/action_goal_handle.c	`58.59% <0.00%> (-1.28%)`	⬇️

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 449575c...c813edc. Read the comment docs.

Acuadros95 · 2022-03-21T07:09:00Z

Nice! Also, CI error is not related to the PR.

JanStaschulat

Great catch. LGTM.

JanStaschulat · 2022-03-21T10:55:00Z

Failing CI build on ROS 2 build farm due to Ubuntu update to jammy

*** No rule to make target '/usr/lib/x86_64-linux-gnu/libpython3.9.so', needed by 'librclc.so'.  Stop.

If not critical, I would wait until that issue is fixed. Otherwise, we could also merge. See related issue: #267

pablogs9 · 2022-03-21T11:20:23Z

I'm ok with waiting

JanStaschulat · 2022-04-01T13:38:32Z

@bjsowa The build jobs on open robotics build farm are running again. However, a change in the ci.yml was also necessary.

You have to options:
A) You could re-do the following changes in ci.yml as in PR https://github.com/ros2/rclc/pull/266/files and push your changes.

B) pull master branch and rebase your PR on master and then push.

Signed-off-by: Błażej Sowa <[email protected]>

pablogs9 · 2022-04-04T05:55:18Z

I have rebased this

pablogs9 · 2022-04-04T06:08:35Z

CI ok, merging

Signed-off-by: Błażej Sowa <[email protected]> (cherry picked from commit 56ee3c1)

mergify · 2022-04-04T06:09:18Z

backport foxy galactic

✅ Backports have been created

#276 rclc_parameter: Fix rcl return values (backport #270) has been created for branch foxy
#277 rclc_parameter: Fix rcl return values (backport #270) has been created for branch galactic

Signed-off-by: Błażej Sowa <[email protected]> (cherry picked from commit 56ee3c1) Co-authored-by: Błażej Sowa <[email protected]>

* rclc_parameter: Fix rcl return values (#270) Signed-off-by: Błażej Sowa <[email protected]> (cherry picked from commit 56ee3c1) * trigger build job on open robotics build farm Signed-off-by: Jan Staschulat (CR/ADA1.2) <[email protected]> * trigger build job on open robotics build farm (2) Signed-off-by: Jan Staschulat (CR/ADA1.2) <[email protected]> Co-authored-by: Błażej Sowa <[email protected]> Co-authored-by: Jan Staschulat (CR/ADA1.2) <[email protected]>

bjsowa force-pushed the master branch from ee78b23 to 745e81a Compare March 18, 2022 16:34

pablogs9 requested review from Acuadros95, JanStaschulat and pablogs9 March 21, 2022 06:47

pablogs9 approved these changes Mar 21, 2022

View reviewed changes

Acuadros95 approved these changes Mar 21, 2022

View reviewed changes

JanStaschulat approved these changes Mar 21, 2022

View reviewed changes

pablogs9 closed this Mar 25, 2022

pablogs9 reopened this Mar 25, 2022

rclc_parameter: Fix rcl return values

c813edc

Signed-off-by: Błażej Sowa <[email protected]>

pablogs9 force-pushed the master branch from 745e81a to c813edc Compare April 4, 2022 05:55

pablogs9 merged commit 56ee3c1 into ros2:master Apr 4, 2022

mergify bot pushed a commit that referenced this pull request Apr 4, 2022

rclc_parameter: Fix rcl return values (#270)

af1cc22

Signed-off-by: Błażej Sowa <[email protected]> (cherry picked from commit 56ee3c1)

mergify bot mentioned this pull request Apr 4, 2022

rclc_parameter: Fix rcl return values (backport #270) #276

Merged

mergify bot pushed a commit that referenced this pull request Apr 4, 2022

rclc_parameter: Fix rcl return values (#270)

4ec4bc3

Signed-off-by: Błażej Sowa <[email protected]> (cherry picked from commit 56ee3c1)

mergify bot mentioned this pull request Apr 4, 2022

rclc_parameter: Fix rcl return values (backport #270) #277

Merged

pablogs9 pushed a commit that referenced this pull request Apr 4, 2022

rclc_parameter: Fix rcl return values (#270) (#277)

3a09087

Signed-off-by: Błażej Sowa <[email protected]> (cherry picked from commit 56ee3c1) Co-authored-by: Błażej Sowa <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rclc_parameter: Fix rcl return values #270

rclc_parameter: Fix rcl return values #270

bjsowa commented Mar 18, 2022

pablogs9 commented Mar 21, 2022

pablogs9 commented Mar 21, 2022

mergify bot commented Mar 21, 2022

pablogs9 left a comment

codecov-commenter commented Mar 21, 2022 •

edited

Loading

Acuadros95 commented Mar 21, 2022 •

edited

Loading

JanStaschulat left a comment

JanStaschulat commented Mar 21, 2022

pablogs9 commented Mar 21, 2022

JanStaschulat commented Apr 1, 2022 •

edited

Loading

pablogs9 commented Apr 4, 2022

pablogs9 commented Apr 4, 2022

mergify bot commented Apr 4, 2022

rclc_parameter: Fix rcl return values #270

rclc_parameter: Fix rcl return values #270

Conversation

bjsowa commented Mar 18, 2022

pablogs9 commented Mar 21, 2022

pablogs9 commented Mar 21, 2022

mergify bot commented Mar 21, 2022

🟠 Waiting for conditions to match

pablogs9 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 21, 2022 • edited Loading

Codecov Report

Acuadros95 commented Mar 21, 2022 • edited Loading

JanStaschulat left a comment

Choose a reason for hiding this comment

JanStaschulat commented Mar 21, 2022

pablogs9 commented Mar 21, 2022

JanStaschulat commented Apr 1, 2022 • edited Loading

pablogs9 commented Apr 4, 2022

pablogs9 commented Apr 4, 2022

mergify bot commented Apr 4, 2022

✅ Backports have been created

codecov-commenter commented Mar 21, 2022 •

edited

Loading

Acuadros95 commented Mar 21, 2022 •

edited

Loading

JanStaschulat commented Apr 1, 2022 •

edited

Loading