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.

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

Bugfix: LifespanQoSExample Using the api incorrectly #4912

Closed
wants to merge 3 commits into from

Conversation

zekai-li
Copy link

@zekai-li zekai-li commented Jun 7, 2024

…simultaneously

LifespanQoSExample Use read_next_sample and take_next_sample simultaneously

Description

LifespanQoSExample Using the api incorrectly,Incorrect read_next_sample and take_next_sample are used at the same time, resulting in the failure to read historical data during the life cycle.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.
  • Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

zekai-li added 3 commits June 7, 2024 16:36
…simultaneously

LifespanQoSExample Use read_next_sample and take_next_sample simultaneously
LifespanQoSExample Using the api incorrectly,Incorrect read_next_sample and take_next_sample are used at the same time, resulting in the failure to read historical data during the life cycle.
LifespanQoSExample Using the api incorrectly,Incorrect read_next_sample and take_next_sample are used at the same time, resulting in the failure to read historical data during the life cycle.
@JesusPoderoso
Copy link
Contributor

Hi @zekai-li, thanks for your contribution.
All the examples are being refactored, as part of the preparation for the following Fast DDS v3.0.0 release.
This example in particular has been condensed in the configuration example.
Therefore introducing these loans changes in the example does not make sense, even for 2.14.x. There will be a specific example using loans in v3.0.0 too.

@zekai-li
Copy link
Author

zekai-li commented Jun 7, 2024

Hi @zekai-li, thanks for your contribution. All the examples are being refactored, as part of the preparation for the following Fast DDS v3.0.0 release. This example in particular has been condensed in the configuration example. Therefore introducing these loans changes in the example does not make sense, even for 2.14.x. There will be a specific example using loans in v3.0.0 too.

I see. Is there a todo list in the v3.0 refactoring? I want to try to submit pr for this project. Also, I think lifespan, deadline, and other fine-grained examples are better for beginners to understand fastdds, which is why I fixed this bug in the first place. Do you want me to add more fine-grained examples to the example?

@zekai-li
Copy link
Author

zekai-li commented Jun 9, 2024

@JesusPoderoso I see. Is there a todo list in the v3.0 refactoring? I want to try to submit pr for this project. thanks

@JesusPoderoso
Copy link
Contributor

Hi @zekai-li!
The list of pending changes is not public, but these are the new requirements for the examples refactor in case you want to contribute to their improvement (also you can take the following pull requests into account as a reference: #4547, #4570, #4836, #4889, #4859, #4913):

  • remove the DDS prefix from the binaries
  • snake case in binary names and directory names
  • take all examples out of the dds folder
  • launch only one endpoint per binary (always running them in different shells)
  • unify args names and utility
  • follow new Application architecture (Application, PublisherApp, SubscriberApp)...
  • write proper README.md with the purpose of the example and expected inputs, outputs, behavior…
  • implement listener callback in the same class (inheritance)
  • include initialization (init()) methods in the constructors, and remove existing init() methods
  • improve parsing options to be a black box to the user

You can start with the RequestReplyExample, which should be renamed to request_reply, or with the SecureHelloWorldExample (renamed to dds_security)

@zekai-li
Copy link
Author

Hi @zekai-li! 你好 ! The list of pending changes is not public, but these are the new requirements for the examples refactor in case you want to contribute to their improvement (also you can take the following pull requests into account as a reference: #4547, #4570, #4836, #4889, #4859, #4913):待定更改的列表不是公开的,但这些是示例重构的新要求,以防您想为示例的改进做出贡献(您也可以考虑以下拉取请求作为参考: #4547#4570#4836#4889#4859#4913 ):

  • remove the DDS prefix from the binaries从二进制文件中删除 DDS 前缀
  • snake case in binary names and directory names二进制名称和目录名称中的蛇形大小写
  • take all examples out of the dds folder从 dds 文件夹中取出所有示例
  • launch only one endpoint per binary (always running them in different shells)每个二进制文件仅启动一个端点(始终在不同的 shell 中运行它们)
  • unify args names and utility统一参数名称和实用程序
  • follow new Application architecture (Application, PublisherApp, SubscriberApp)...遵循新的应用程序架构(应用程序、PublisherApp、SubscriberApp)...
  • write proper README.md with the purpose of the example and expected inputs, outputs, behavior…根据示例的目的和预期的输入、输出、行为编写正确的 README.md...
  • implement listener callback in the same class (inheritance)在同一个类中实现监听器回调(继承)
  • include initialization (init()) methods in the constructors, and remove existing init() methods在构造函数中包含初始化 ( init() ) 方法,并删除现有的 init() 方法
  • improve parsing options to be a black box to the user改进解析选项,使其成为用户的黑匣子

You can start with the RequestReplyExample, which should be renamed to request_reply, or with the SecureHelloWorldExample (renamed to dds_security)您可以从 RequestReplyExample 开始,它应重命名为 request_reply ,或者从 SecureHelloWorldExample 开始(重命名为 dds_security

Thanks for your detailed response, I will submit the code for our community as soon as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants