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

iox-#14 replace ChunkInfo with ChunkHeader members #438

Merged
merged 8 commits into from
Dec 17, 2020
Merged

iox-#14 replace ChunkInfo with ChunkHeader members #438

merged 8 commits into from
Dec 17, 2020

Conversation

elBoberido
Copy link
Member

@elBoberido elBoberido commented Dec 15, 2020 •
edited
Loading

Signed-off-by: Mathias Kraus [email protected]

Pre-Review Checklist for the PR Author

  1. Branch follows the naming format (iox-#123-this-is-a-branch)
  2. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  3. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  4. Relevant issues are linked
  5. Add sensible notes for the reviewer
  6. All checks have passed
  7. Assign PR to reviewer

Notes for Reviewer

This PR replaces ChunkInfo with members from ChunkHeader and moves some types to iceoryx_posh_types.hpp

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

  1. All checkboxes in the PR checklist are checked or crossed out
  2. Merge

References

elBoberido
elBoberido commented Dec 15, 2020
View reviewed changes
iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp
Comment on lines +202 to +208
using BaseClock_t = std::chrono::steady_clock;

// use signed integer for duration;
// there is a bug in gcc 4.8 which leads to a wrong calcutated time
// when sleep_until() is used with a timepoint in the past
using DurationNs_t = std::chrono::duration<std::int64_t, std::nano>;
using TimePointNs_t = std::chrono::time_point<BaseClock_t, DurationNs_t>;
Copy link
Member Author

Choose a reason for hiding this comment

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

these could probably also fit into the plain iox namespace. It's a bit weird to have a DurationNs_t which is not based on cxx::Duration though

Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove them completely and should always use units::duration.

Copy link
Member Author

Choose a reason for hiding this comment

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

so, no chrono based timepoints?

Copy link
Member

Choose a reason for hiding this comment

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

Is the issue in cxx::Duration with the rounding error resolved? For example when you use 1 ns then its represented as zero in the duration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not, it's still using a double. @elfenpiff will receive the honour I suppose :P

@dkroenke dkroenke self-requested a review December 16, 2020 16:27
dkroenke
dkroenke previously approved these changes Dec 16, 2020
View reviewed changes
@elBoberido
iox-#14 add U postfix to literals
2ba9ecc 
Signed-off-by: Mathias Kraus <[email protected]>
@elBoberido elBoberido force-pushed the iox-#14-remove-ChunkInfo branch from 2469ae8 to 2ba9ecc Compare December 16, 2020 16:47
@budrus budrus self-requested a review December 16, 2020 17:04
budrus
budrus previously approved these changes Dec 16, 2020
View reviewed changes
budrus
budrus previously approved these changes Dec 17, 2020
View reviewed changes
mossmaurice
mossmaurice requested changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@mossmaurice mossmaurice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please try to avoid using chrono directly. The advantage of using cxx::Duration is that we have conversion operators to the commonly used timeval and timespec. I suppose the best idea is to use chrono inside cxx::Duration

iceoryx_posh/source/mepoo/chunk_header.cpp Show resolved Hide resolved
iceoryx_posh/test/moduletests/test_mepoo_chunk_header.cpp Outdated Show resolved Hide resolved
@elBoberido
iox-#14 fix review findigs
78dea37 
Signed-off-by: Mathias Kraus <[email protected]>
@elBoberido
Copy link
Member Author

@mossmaurice the usage of chrono is not new but legacy code. It was used for the timestamp in the ChunkInfo and is still used for the timestamp of the last heartbeat of the application. We don't have something to replace that. If the decision is to get rid of chrono, I would create an issue for that.

@mossmaurice
Copy link
Contributor

@mossmaurice the usage of chrono is not new but legacy code. It was used for the timestamp in the ChunkInfo and is still used for the timestamp of the last heartbeat of the application. We don't have something to replace that. If the decision is to get rid of chrono, I would create an issue for that.

Fine by me. @budrus What about tackling this topic in v1.x? Or is chrono and cxx::Duration used in a public API?

@elBoberido
Copy link
Member Author

@mossmaurice chrono is not user facing. I'm not sure about cxx::Duration, but I think we use it everywhere a timeout is specified

@budrus budrus self-requested a review December 17, 2020 12:39
@budrus
Copy link
Contributor

budrus commented Dec 17, 2020

@mossmaurice the usage of chrono is not new but legacy code. It was used for the timestamp in the ChunkInfo and is still used for the timestamp of the last heartbeat of the application. We don't have something to replace that. If the decision is to get rid of chrono, I would create an issue for that.

Fine by me. @budrus What about tackling this topic in v1.x? Or is chrono and cxx::Duration used in a public API?

Does it makes sense to address this together with #337?

@elBoberido
Copy link
Member Author

@mossmaurice the usage of chrono is not new but legacy code. It was used for the timestamp in the ChunkInfo and is still used for the timestamp of the last heartbeat of the application. We don't have something to replace that. If the decision is to get rid of chrono, I would create an issue for that.

Fine by me. @budrus What about tackling this topic in v1.x? Or is chrono and cxx::Duration used in a public API?

Does it makes sense to address this together with #337?

yes, I added it there

@elBoberido elBoberido merged commit 4ce57b0 into eclipse-iceoryx:master Dec 17, 2020
@elBoberido elBoberido deleted the iox-#14-remove-ChunkInfo branch December 18, 2020 14:34
dkroenke added a commit to ApexAI/iceoryx that referenced this pull request Feb 11, 2021
@dkroenke
iox-eclipse-iceoryx#438 ament_copyright check
6cb28e9 
Signed-off-by: Dietrich Krönke <[email protected]>
dkroenke added a commit to ApexAI/iceoryx that referenced this pull request Feb 11, 2021
@dkroenke
iox-eclipse-iceoryx#438 let colcon build
6b8c071 
Signed-off-by: Dietrich Krönke <[email protected]>
dkroenke added a commit to ApexAI/iceoryx that referenced this pull request Feb 11, 2021
@dkroenke
iox-eclipse-iceoryx#438 fix path setting in colcon build
21e4e07 
Signed-off-by: Dietrich Krönke <[email protected]>
dkroenke added a commit to ApexAI/iceoryx that referenced this pull request Feb 11, 2021
@dkroenke
iox-eclipse-iceoryx#438 use sudo for patching ament_copyright templat…
4128a1e 
…e in Github CI

Signed-off-by: Dietrich Krönke <[email protected]>
dkroenke added a commit to ApexAI/iceoryx that referenced this pull request Feb 11, 2021
@dkroenke
iox-eclipse-iceoryx#438 delete template before copying it into ROS2
15f177d 
Signed-off-by: Dietrich Krönke <[email protected]>
dkroenke added a commit to ApexAI/iceoryx that referenced this pull request Feb 11, 2021
@dkroenke
iox-eclipse-iceoryx#438 adapt cp statement for copyright template
ce756a6 
Signed-off-by: Dietrich Krönke <[email protected]>
dkroenke added a commit to ApexAI/iceoryx that referenced this pull request Feb 11, 2021
@dkroenke
iox-eclipse-iceoryx#438 ament_copyright check
2939ac4 
Signed-off-by: Dietrich Krönke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elfenpiff elfenpiff elfenpiff left review comments

@FerdinandSpitzschnueffler FerdinandSpitzschnueffler FerdinandSpitzschnueffler left review comments

@mossmaurice mossmaurice mossmaurice approved these changes

@budrus budrus budrus approved these changes

@dkroenke dkroenke dkroenke left review comments

@MatthiasKillat MatthiasKillat Awaiting requested review from MatthiasKillat

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

Successfully merging this pull request may close these issues.

Reserved chunkinfo user payload header
6 participants
@elBoberido @mossmaurice @budrus @dkroenke @elfenpiff @FerdinandSpitzschnueffler