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

Prototype of fragmented message support. #11

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

smoriemb
Copy link

@smoriemb smoriemb commented Jan 3, 2023

This pull request is to add a fragmented data sending feature to embeddedRTPS.

I added a send-message-interface newChangeCallback() receiving a function object instead of a pointer to a buffer that includes a message to be sent (like the original "newChange()") .

The function object is expected to serialize one fragmented data each time embeddedRTPS sent the fragmented data to the lwIP layer.
The type of the function object has a return type, as below.

std::pair<uint8_t *, DataSize_t> 

where the first is the pointer to the fragmented data and the second is the size of the data. If there is no data to be fragmented, the function object returns zero size and embeddedRTPS perceives the end of the fragmented data.

The sender thread (progress()) of embeddedRTPS checks if each history cache has the function object, or not. If yes, it treat the history cache as a fragmented data and calls the function object. If no, it treat it as a non-fragmented data and do the exsiting procedure.

Limitation:

  • Statefull Write is not supported.

@takasehideki
Copy link
Member

takasehideki commented Jan 5, 2023

@smoriemb Thank you so much for your awesome contribution as "OTOSHIDAMA"!! 🙄
There are 4 concerns for now.


In my understanding, this PR can be checked with https://github.com/smoriemb/mros2-mbed/tree/feature/mros2-frag-msg-proto that was mentioned at mROS-base/mros2-mbed#32. And also, this PR needs to add changes to mros2 and mros2-mbed repositories. Are they correct?
mROS-base/mros2@main...smoriemb:mros2:feature/mros2-frag-msg-proto
mROS-base/mros2-mbed@main...smoriemb:mros2-mbed:feature/mros2-frag-msg-proto

Could you also create PRs to mros2 and mros2-mbed repositories that associate with this PR?


I'm so curious about the Limitation: Statefull Write is not supported. However, I cannot properly understand how this limitation affects the product.
If possible, could you explain whether this limitation is problematic or not, or, how it needs to be addressed in the future?


In my opinion (or strategy), I want to locate only std_msgs in mros2 repository, because I want to keep the size and responsibility of the library repository as small as possible, but std_msgs will be used so frequently (it is acceptable to standardize this only).
You have added sensor_msgs in it.
mROS-base/mros2@main...smoriemb:mros2:feature/mros2-frag-msg-proto#diff-0782be83e57ac4d4cdde6899cf0039a6259f835ba8ee25ad0f27538e8ce5b2a7

If you can agree with my opinion, could you move sensor_msgs into mros2-mbed repository?


My understanding is that you want to create the very long string from README.md by the below method.
mROS-base/mros2-mbed@main...smoriemb:mros2-mbed:feature/mros2-frag-msg-proto#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a

But I think that we simply need to prepare long_text.txt for this requirement. And also, I think that modifying CMakeLists.txt will need to be maintained for future use.
Please simply prepare long_text.txt?

@smoriemb
Copy link
Author

smoriemb commented Jan 5, 2023

And also, this PR needs to add changes to mros2 and mros2-mbed repositories. Are they correct?

In my understanding, the PR works well without the other PRs because I added only an interface that is not used by the existing mROS2.

Could you also create PRs to mros2 and mros2-mbed repositories that associate with this PR?

I've created the related pull-requests below.
[mROS2]
mROS-base/mros2#36
[mROS2 on Mbed]
mROS-base/mros2-mbed#33
[Host example]
mROS-base/mros2-host-examples#14
But some of these have submodules stored in my repositories because I think it is useful to check if they work. If not, could you kindly point it out so that I can modify the reference to each repository?

I'm so curious about the Limitation: Statefull Write is not supported. However, I cannot properly understand how this limitation affects the product.
If possible, could you explain whether this limitation is problematic or not, or, how it needs to be addressed in the future?

The limitation does not affect the existing mROS2 because the Stateful writer is used only for the discovery system. However, if mROS2 extends its feature to use Stateful writer for normal messages, we will have to consider how we can extend the fragmented feature to use for reliable communication.

If you can agree with my opinion, could you move sensor_msgs into mros2-mbed repository?

I've already added the message type in the above pull-request of mROS2.

But I think that we simply need to prepare long_text.txt for this requirement. And also, I think that modifying CMakeLists.txt will need to be maintained for future use.
Please simply prepare long_text.txt?

Could you kindly continue to discuss this in the thread below?
mROS-base/mros2-mbed#33 (comment)

@takasehideki
Copy link
Member

Thank you so much for your kind operations. I will check your PRs. I knew you were right about all the updates being included in this PR, but now it is easier to review each update and I can merge each of them at once if there are no problems.
And thanks for the explanation about Stateful Writer. I think it's fine as it is now, but I'd like to make a note of it somewhere.

We will review them ASAP!

@takasehideki
Copy link
Member

@smoriemb long, long, and seriously long time no see,,,
We were finally able to confirm that this and related PRs work fine in our environment. We deeply apologize for the long neglect.

We will merge your fantastic work in due course!

@takasehideki takasehideki merged commit e6ddfec into mROS-base:mros2 Apr 11, 2023
takasehideki added a commit that referenced this pull request Apr 11, 2023
add note about PR #11, that is the specific modification for mros2
kazu-321 pushed a commit to kazu-321/mros2-mbed-studio-embeddedRTPS that referenced this pull request Mar 31, 2024
It involked a warning message when we use CMake environment.

```
./embeddedRTPS/src/entities/StatelessReader.cpp:73:8: warning: extra tokens at end of #endif directive [-Wendif-labels]
   73 | #endif SLR_VERBOSE
```
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.

None yet

2 participants