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

[22682] Add RPCDDS internal API #5638

Merged
merged 33 commits into from
Mar 5, 2025

Conversation

Carlosespicur
Copy link
Contributor

@Carlosespicur Carlosespicur commented Feb 10, 2025

Description

This PR adds a set of classes that represent all entities required to establish an RPC over DDS Request/Reply communication. All RPC entities should be created from DomainParticipant, so a list of methods has been implemented for this purpose. The main additions are detailed below:

  • Requester/Replier/Service interfaces and implementations added. Each Service represent the subset of DDS entities (Topic/Publisher/DataWriter/Subscriber/DataReader) required to establish an RPC Request Reply communication. When a Service is instantiated by a DomainParticipant, Request and Reply topics are created. Each service can instantiate Requester and Replier entities, which are internally registered for entity ownership checking and memory managing. When a Requester is instantiated by a service, a Request Topic DataWriter and Reply Topic DataReader are created. When a Reply is instantiated by a Service, a Request Topic DataReader and Reply Topic DataWriter are created. Both Service and Requester/Replier entities have internal methods to check if all DDS entities are correctly created.

  • Requester and Replier objects are created from an instance of RequesterQos and ReplierQos classes respectively. These objects contain the parameters used to configure a Requester/Replier. Before instantiation, a parameter validation at Service level is made. In RPC context, dds entities only accept RELIABLE_RELIABILITY_QOS for reliability QoS.

  • Requester/Replier/Service public APIs are represented with abstract classes. Requester/Replier/Service implementation classes inherit from them and implement their own set of methods used for DDS entities creation and internal registering/checking processes.

  • Content filtering: Reply samples sent by a replier are filtered by participant using the SampleIdentity's GuidPrefix of its associated request sample. It could be improved adding an internal common identifier for all DDS entities on a given Requester

  • Type registering: A ServiceTypeSupport interface has been added for registering service types. In the Request/Reply context, a service type is represented as a pair of topic types (request topic type and reply topic type), so register a service type consists on register both types in DomainParticipant. Registered service types are stored in DomainParticipant and can be accessed by name. register_service_type and unregister_service_type methods have been implemented on DomainParticipant public API

  • RPC communication methods: Request and Replier public APIs have methods for sending and taking samples (send_request, send_reply, take_request, take_reply). Send methods call DataWriter write method and Take methods call DataReader take method. The additional information of each request sample sent is represented by RequestInfo class.

  • Testing: RPC Blackbox tests refactored to use the new API. Unit tests for service and DomainParticipant added.

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.
  • N/A: Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • NO: 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.
  • N/A: 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.
  • If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@Carlosespicur Carlosespicur changed the base branch from master to feature/rpc/main February 10, 2025 13:39
@Carlosespicur Carlosespicur force-pushed the feature/rpc/requester-replier-fixed branch from 58631df to 6c708b5 Compare February 10, 2025 15:55
@Carlosespicur Carlosespicur marked this pull request as ready for review February 11, 2025 07:26
@Carlosespicur Carlosespicur added the doc-pending Issue or PR which is pending to be documented label Feb 11, 2025
Signed-off-by: Carlosespicur <[email protected]>
@github-actions github-actions bot added the ci-pending PR which CI is running label Feb 12, 2025
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Just some initial architectural concerns

@Carlosespicur Carlosespicur force-pushed the feature/rpc/requester-replier-fixed branch from 77236b4 to 5dc3dd5 Compare February 28, 2025 13:00
@MiguelCompany MiguelCompany self-requested a review March 3, 2025 14:46
@MiguelCompany MiguelCompany self-requested a review March 3, 2025 15:09
@Carlosespicur Carlosespicur force-pushed the feature/rpc/requester-replier-fixed branch 5 times, most recently from b2dded8 to c07201f Compare March 4, 2025 13:30
Signed-off-by: Carlosespicur <[email protected]>
@Carlosespicur Carlosespicur force-pushed the feature/rpc/requester-replier-fixed branch from c07201f to 6285bf7 Compare March 4, 2025 13:32
@MiguelCompany MiguelCompany requested review from MiguelCompany and removed request for MiguelCompany March 4, 2025 15:11
@Carlosespicur Carlosespicur requested review from MiguelCompany and removed request for MiguelCompany March 5, 2025 10:14
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@MiguelCompany
Copy link
Member

Failures unrelated. Going in!

@MiguelCompany MiguelCompany merged commit b8856b4 into feature/rpc/main Mar 5, 2025
15 of 17 checks passed
@MiguelCompany MiguelCompany deleted the feature/rpc/requester-replier-fixed branch March 5, 2025 15:26
MiguelCompany pushed a commit that referenced this pull request Mar 5, 2025
* Refs #22682: Add public interfaces of RPC Entities

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add RequestInfo class

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add ServiceTypeSupport interface

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add Requester/Replier parameter classes

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add private headers and implement RPC methods

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Expose Requester/Replier endpoint getters to public API

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add public methods for deleting requester/replier entities

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Allow calling get_statuscondition method on constant entities

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Refactor blackbox tests

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Avoid sample filtering on request topic

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix segFault error due to deleting endpoints manually

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix TCPRequester params

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Enable type() method for const DataReader objects

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add unit tests for Requester/Replier parameters validation

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add unit tests for DomainParticipant public API methods

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix test errors. Add service/participant checks and avoid inconsistent statuses when unregister types methods fail

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Update versions.md

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes. Remove RequesterParams/ReplierParams classes

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes. Unify parameters in Requester/Replier take/send methods

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix RequesterImpl::send_request related_sample_identity assignment

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix typos

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Uncrustify

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Avoid creating unnecesary RequestReplyContentFilter instances

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Add suggested changes

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes. Add return_load method to Requester/Replier public API

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes. Blackbox tests classes

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Apply suggested changes. Update send_reply signature.

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix blackbox tests

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix notation errors in Requester/Replier

Signed-off-by: Carlosespicur <[email protected]>

* Refs #22682: Fix memory leaks in RequestReplyContentFilterFactory

Signed-off-by: Carlosespicur <[email protected]>

---------

Signed-off-by: Carlosespicur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-pending PR which CI is running
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants