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

add rmw_get_default_xxx to return the structure with default values. #379

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
+78 −9
Open

add rmw_get_default_xxx to return the structure with default values. #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions rmw/include/rmw/event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ RMW_WARN_UNUSED
rmw_event_t
rmw_get_zero_initialized_event(void);

/// Return a default event structure.
RMW_PUBLIC
RMW_WARN_UNUSED
rmw_event_t
rmw_get_default_event(void);
Comment on lines +71 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

For instance, I don't believe we need this. In all "real" situations (not tests) where rmw_get_zero_initialized_event is called, it is immediately followed by either rmw_subscription_event_init or rmw_publisher_event_init. Because of that, I don't believe we need to add this API at all (though if we change rmw_get_zero_initialized_event to truly return all zeros, we may have to fix a couple of tests).


/// Initialize a rmw publisher event.
/**
* \param[inout] rmw_event to initialize
Expand Down
6 changes: 6 additions & 0 deletions rmw/include/rmw/init.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ RMW_WARN_UNUSED
rmw_context_t
rmw_get_zero_initialized_context(void);

/// Return a default context structure.
RMW_PUBLIC
RMW_WARN_UNUSED
rmw_context_t
rmw_get_default_context(void);
Comment on lines +58 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here; all places we call rmw_get_zero_initialized_context we are either cleaning up after a failure, doing fini on the context, or this is immediately followed by an rmw_init which initializes the context. Again, we may have to fix some tests if we make rmw_get_zero_initialized_context actually return all zeros.


/// Initialize the middleware with the given options, and yielding an context.
/**
* Context is filled with middleware specific data upon success of this function.
Expand Down
6 changes: 6 additions & 0 deletions rmw/include/rmw/init_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ RMW_WARN_UNUSED
rmw_init_options_t
rmw_get_zero_initialized_init_options(void);

/// Return a default init options structure.
RMW_PUBLIC
RMW_WARN_UNUSED
rmw_init_options_t
rmw_get_default_init_options(void);

Comment on lines +74 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

And the same here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@clalancette i take this case as an example. RMW_DEFAULT_DOMAIN_ID is not zero, so i was wondering that it should be returned by rmw_get_default_init_options originally. if we truly return the zero initialized option, this will affect the user application. i think we can have some options,

  • get_zero_initialized_xxx returns with default values. (almost current implementation) IMO this does not sound right, because what it does is not really what it sounds like... initialized_xxx would sound better.
  • get_zero_initialized_xxx truly returns with zero initialized fields, and provides APIs to get default values too. (these PRs)
  • get_zero_initialized_xxx truly returns with zero initialized fields, and caller is responsible to fill in default values. (this does not really make sense, this is exactly whey get_default_xxx suggested.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely think that get_zero_initialized_xxx should return a zero-initialized structure. So we are in agreement there.

But my main point is that all callers of get_zero_initialized_xxx (with the exception of maybe a few tests) immediately follow that up with a corresponding init_xxx call, which basically sets the defaults. For instance, considering the rmw_init_options_t structure, we have rmw_get_zero_initialized_init_options, which I believe should return a zero'ed structure. But all callers immediately follow that with a call to rmw_init_options_init. That function is implemented by the various RMWs:

  • rmw_fastrtps - initializes the structure very much like your proposed change here
  • rmw_cyclonedds - initializes the structure very much like your proposed change here
  • rmw_connextdds - initializes the structure very much like your proposed change here
  • rmw_zenoh - initializes the structure very much like your proposed change here

Thus, it doesn't seem like the new APIs are necessary, as we already have all of the functionality we need here.

To maybe ask this in a different way: in what situations are you expecting that rmw_get_default_init_options will be used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I definitely think that get_zero_initialized_xxx should return a zero-initialized structure. So we are in agreement there.

👍

To maybe ask this in a different way: in what situations are you expecting that rmw_get_default_init_options will be used?

ah, i see now. i was thinking rmw_get_default_init_options returns implementation agnostic default option from ROS 2 RMW (e.g ROS 2 default domain ID), and rmw_init_options_init initializes the field with implementation specific values. but your point is that is not how it is implemented, so calling implementation rmw_init_options_init also initializes with default values.

i can check the related APIs to make sure, and adjust the PR accordingly. thanks!

/// Initialize given init options with the default values and implementation specific values.
/**
* The given allocator is used, if required, during setup of the init options,
Expand Down
12 changes: 8 additions & 4 deletions rmw/src/event.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,15 @@ extern "C" {
rmw_event_t
rmw_get_zero_initialized_event(void)
{
// TODO(@fujitatomoya): This is not exatly zero initialized structure.
/// We should introduce xxx_get_default_event to return the default values.
// All members are initialized to 0 or NULL by C99 6.7.8/10.
static const rmw_event_t event;
return event;
}

rmw_event_t
rmw_get_default_event(void)
{
static const rmw_event_t event = {
.implementation_identifier = NULL,
.data = NULL,
.event_type = RMW_EVENT_INVALID
};
return event;
Expand Down
10 changes: 9 additions & 1 deletion rmw/src/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,19 @@ extern "C"

rmw_context_t
rmw_get_zero_initialized_context(void)
{
// All members are initialized to 0 or NULL by C99 6.7.8/10.
static const rmw_context_t context;
return context;
}

rmw_context_t
rmw_get_default_context(void)
{
return (const rmw_context_t) {
.instance_id = 0,
.implementation_identifier = NULL,
.options = rmw_get_zero_initialized_init_options(),
.options = rmw_get_default_init_options(),
.actual_domain_id = 0u,
.impl = NULL
}; // NOLINT(readability/braces): false positive
Expand Down
10 changes: 8 additions & 2 deletions rmw/src/init_options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,14 @@ extern "C"
rmw_init_options_t
rmw_get_zero_initialized_init_options(void)
{
// TODO(@fujitatomoya): This is not exatly zero initialized structure.
/// We should introduce xxx_get_default_init_optionst to return the default values.
// All members are initialized to 0 or NULL by C99 6.7.8/10.
static const rmw_init_options_t init_option;
return init_option;
}

rmw_init_options_t
rmw_get_default_init_options(void)
{
static const rmw_init_options_t init_option = {
.domain_id = RMW_DEFAULT_DOMAIN_ID,
.discovery_options = {RMW_AUTOMATIC_DISCOVERY_RANGE_NOT_SET, 0},
Expand Down
10 changes: 9 additions & 1 deletion rmw/test/test_event.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ TEST(rmw_event, get_zero_initialized_event)
const rmw_event_t actual = rmw_get_zero_initialized_event();
EXPECT_EQ(nullptr, actual.implementation_identifier);
EXPECT_EQ(nullptr, actual.data);
EXPECT_EQ(0u, actual.event_type);
}

TEST(rmw_event, get_default_event)
{
const rmw_event_t actual = rmw_get_default_event();
EXPECT_EQ(nullptr, actual.implementation_identifier);
EXPECT_EQ(nullptr, actual.data);
EXPECT_EQ(RMW_EVENT_INVALID, actual.event_type);
}

Expand All @@ -36,5 +44,5 @@ TEST(rmw_event, event_fini)
EXPECT_EQ(rmw_event_fini(&event), RMW_RET_OK);
EXPECT_EQ(nullptr, event.implementation_identifier);
EXPECT_EQ(nullptr, event.data);
EXPECT_EQ(RMW_EVENT_INVALID, event.event_type);
EXPECT_EQ(0u, event.event_type);
}
15 changes: 14 additions & 1 deletion rmw/test/test_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,22 @@
#include "gmock/gmock.h"
#include "rmw/init.h"

TEST(rmw_init_options, get_zero_initialized_init_options)
TEST(rmw_init_options, get_zero_initialized_context)
{
const rmw_context_t context = rmw_get_zero_initialized_context();
EXPECT_EQ(context.instance_id, 0u);
EXPECT_EQ(context.implementation_identifier, nullptr);
EXPECT_EQ(context.options.domain_id, 0u);
EXPECT_EQ(context.actual_domain_id, 0u);
EXPECT_EQ(context.impl, nullptr);
}

TEST(rmw_init_options, get_default_context)
{
const rmw_context_t context = rmw_get_default_context();
EXPECT_EQ(context.instance_id, 0u);
EXPECT_EQ(context.implementation_identifier, nullptr);
EXPECT_EQ(context.options.domain_id, RMW_DEFAULT_DOMAIN_ID);
EXPECT_EQ(context.actual_domain_id, 0u);
EXPECT_EQ(context.impl, nullptr);
}
12 changes: 12 additions & 0 deletions rmw/test/test_init_options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,19 @@
TEST(rmw_init_options, get_zero_initialized_init_options)
{
const rmw_init_options_t options = rmw_get_zero_initialized_init_options();
EXPECT_EQ(options.domain_id, 0u);
EXPECT_EQ(options.instance_id, 0u);
EXPECT_EQ(options.implementation_identifier, nullptr);
EXPECT_EQ(options.impl, nullptr);
EXPECT_EQ(options.enclave, nullptr);
}

TEST(rmw_init_options, get_default_init_options)
{
const rmw_init_options_t options = rmw_get_default_init_options();
EXPECT_EQ(options.domain_id, RMW_DEFAULT_DOMAIN_ID);
EXPECT_EQ(options.instance_id, 0u);
EXPECT_EQ(options.implementation_identifier, nullptr);
EXPECT_EQ(options.impl, nullptr);
EXPECT_EQ(options.enclave, nullptr);
}