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.

Sign up for GitHub

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

Jump to bottom

Parameters fini memory #253

Merged
merged 6 commits into from
Mar 4, 2022
Merged

Parameters fini memory #253

merged 6 commits into from
Mar 4, 2022

Conversation

pablogs9
Copy link
Member

Signed-off-by: Pablo Garrido [email protected]

@pablogs9 pablogs9 requested a review from Acuadros95 February 28, 2022 13:51
@pablogs9
Copy link
Member Author

Not related error:
image

We should check

@JanStaschulat
Copy link
Contributor

It is related to the PR of the events executor in RMW:
#254

@JanStaschulat
Copy link
Contributor

JanStaschulat commented Mar 2, 2022 •
edited
Loading

@pablogs9 the PR #255 is merged. You can do a rebase. Then the CI RCLC Rolling build job should pass.

JanStaschulat
JanStaschulat approved these changes Mar 2, 2022
View reviewed changes
Copy link
Contributor

@JanStaschulat JanStaschulat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

pablogs9 added 3 commits March 2, 2022 12:13
@pablogs9
Parameters fini memory
18792eb 
Signed-off-by: Pablo Garrido <[email protected]>
@pablogs9
Uncrustify
4b0914d 
Signed-off-by: Pablo Garrido <[email protected]>
@pablogs9
Update
1a2391e 
Signed-off-by: Pablo Garrido <[email protected]>
@pablogs9 pablogs9 force-pushed the fix/parameters_fini branch from c27a550 to 1a2391e Compare March 2, 2022 11:13
@pablogs9
Copy link
Member Author

pablogs9 commented Mar 2, 2022

@mergify backport galactic

@mergify
Copy link
Contributor

mergify bot commented Mar 2, 2022

backport galactic

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]

Hey, I reacted but my real name is @Mergifyio

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2022 •
edited
Loading

Codecov Report

Merging #253 (c41e7f4) into master (4f4590f) will increase coverage by 0.99%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   63.42%   64.42%   +0.99%     
==========================================
  Files          16       16              
  Lines        2149     2184      +35     
  Branches      641      647       +6     
==========================================
+ Hits         1363     1407      +44     
+ Misses        472      460      -12     
- Partials      314      317       +3
Impacted Files Coverage Δ
...lc_parameter/src/rclc_parameter/parameter_server.c 72.47% <100.00%> (+5.61%) ⬆️

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 4f4590f...c41e7f4. Read the comment docs.

pablogs9 added 3 commits March 2, 2022 12:29
@pablogs9
Add fini test
027fb92 
Signed-off-by: Pablo Garrido <[email protected]>
@pablogs9
Add fini test
06fb02e 
Signed-off-by: Pablo Garrido <[email protected]>
@pablogs9
Fix destruction
c41e7f4 
Signed-off-by: Pablo Garrido <[email protected]>
@pablogs9
Copy link
Member Author

pablogs9 commented Mar 2, 2022

I have this passing in local and Rpr__rclc__ubuntu_jammy_amd64 is also ok. Any idea of the timeout in CI RCLC Rolling / build (ubuntu-20.04, rolling) @JanStaschulat ?

@pablogs9
Copy link
Member Author

pablogs9 commented Mar 2, 2022

Same timeout here: #257

@pablogs9
Copy link
Member Author

pablogs9 commented Mar 3, 2022

Today's CI is passing, are we ok with that? @JanStaschulat @Acuadros95 can we assume that this is a CI or Action failure?

@pablogs9 pablogs9 mentioned this pull request Mar 3, 2022
Closed
@JanStaschulat
Copy link
Contributor

I suppose it was an issue at the Github Action. I am fine with merging.

@pablogs9 pablogs9 merged commit 3e5e219 into master Mar 4, 2022
@pablogs9 pablogs9 deleted the fix/parameters_fini branch March 4, 2022 08:20
mergify bot pushed a commit that referenced this pull request Mar 4, 2022
@pablogs9
Parameters fini memory (#253)
d2bb539 
* Parameters fini memory

Signed-off-by: Pablo Garrido <[email protected]>

* Uncrustify

Signed-off-by: Pablo Garrido <[email protected]>

* Update

Signed-off-by: Pablo Garrido <[email protected]>

* Add fini test

Signed-off-by: Pablo Garrido <[email protected]>

* Add fini test

Signed-off-by: Pablo Garrido <[email protected]>

* Fix destruction

Signed-off-by: Pablo Garrido <[email protected]>
(cherry picked from commit 3e5e219)
@mergify mergify bot mentioned this pull request Mar 4, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Mar 4, 2022

backport galactic

✅ Backports have been created

pablogs9 added a commit that referenced this pull request Mar 4, 2022
@mergify @pablogs9
Parameters fini memory (#253) (#261)
d5ca470 
* Parameters fini memory

Signed-off-by: Pablo Garrido <[email protected]>

* Uncrustify

Signed-off-by: Pablo Garrido <[email protected]>

* Update

Signed-off-by: Pablo Garrido <[email protected]>

* Add fini test

Signed-off-by: Pablo Garrido <[email protected]>

* Add fini test

Signed-off-by: Pablo Garrido <[email protected]>

* Fix destruction

Signed-off-by: Pablo Garrido <[email protected]>
(cherry picked from commit 3e5e219)

Co-authored-by: Pablo Garrido <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanStaschulat JanStaschulat JanStaschulat approved these changes

@Acuadros95 Acuadros95 Awaiting requested review from Acuadros95

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pablogs9 @JanStaschulat @codecov-commenter