[#550] custom event ids

[elfenpiff]

Notes for Reviewer

Pre-Review Checklist for the PR Author

• Add sensible notes for the reviewer
• PR title is short, expressive and meaningful
• Consider switching the PR to a draft (Convert to draft)
  • as draft PR, the CI will be skipped for pushes
• Relevant issues are linked in the References section
• Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
• Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
• Commits messages are according to this guideline
  • Commit messages have the issue ID ([#123] Add posix ipc example)
  • Commit author matches Eclipse Contributor Agreement(and ECA is signed)
• Tests follow the best practice for testing
• Changelog updated in the unreleased section including API breaking changes
• Assign PR to reviewer
• All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Unit tests have been written for new behavior
• Public API is documented
• PR title describes the changes

Post-review Checklist for the PR Author

• All open points are addressed and tracked via issues

References

Closes #550

[elBoberido]

Is this intentional to deviate from the Rust API or just an overlook?

[elfenpiff]

This is intentional. In the builder it is no longer an optional but for the global config and the config file it is an optional. Some = emit event, None = no event.

[elBoberido]

Wouldn't it make sense to have the same API with disable?

[elfenpiff]

No. In Rust the config is a struct where all members are public and can be set by the user directly. It is also serializable and stored in the iceoryx2.toml file. This config entry is still an Option<EventId> and it does not make sense to split it up into two fields since then we confuse the user and may have entries that are in contradiction to each other.

We require here a C++ API since we cannot access the struct members directly in C++. But the API shall reflect the underlying structure of the Rust structs/members. Therefore this method has still an optional.

[elBoberido]

Suggested change

	/// If the [`Service`] is created it defines the event that shall be emitted by every newly
	/// created [`Notifier`].
	IOX_BUILDER_OPTIONAL(EventId, notifier_created_event);
	/// If the [`Service`] is created it disables sending an event when a new notifier was created.
	IOX_BUILDER_SWITCH(disable_notifier_created_event);
	/// If the [`Service`] is created it defines the event that shall be emitted by every
	/// [`Notifier`] before it is dropped. If [`None`] is
	/// provided a [`Notifier`] will not emit an event.
	IOX_BUILDER_OPTIONAL(EventId, notifier_dropped_event);
	/// If the [`Service`] is created it disables sending an event when a notifier was dropped.
	IOX_BUILDER_SWITCH(disable_notifier_dropped_event);
	/// If the [`Service`] is created it defines the event that shall be emitted when a
	/// [`Notifier`] is identified as dead. If [`None`] is
	/// provided no event will be emitted.
	IOX_BUILDER_OPTIONAL(EventId, notifier_dead_event);
	/// If the [`Service`] is created it disables sending an event when a notifier was identified
	/// as dead.
	IOX_BUILDER_SWITCH(disable_notifier_dead_event);
	/// If the [`Service`] is created it defines the event that shall be emitted by every newly
	/// created [`Notifier`].
	IOX_BUILDER_OPTIONAL(EventId, notifier_created_event);
	/// If the [`Service`] is created it defines the event that shall be emitted by every
	/// [`Notifier`] before it is dropped. If [`None`] is
	/// provided a [`Notifier`] will not emit an event.
	IOX_BUILDER_OPTIONAL(EventId, notifier_dropped_event);
	/// If the [`Service`] is created it defines the event that shall be emitted when a
	/// [`Notifier`] is identified as dead. If [`None`] is
	/// provided no event will be emitted.
	IOX_BUILDER_OPTIONAL(EventId, notifier_dead_event);
	/// If the [`Service`] is created it disables sending an event when a new notifier was created.
	void disable_notifier_created_event() {
	m_notifier_created_event.reset();
	}
	/// If the [`Service`] is created it disables sending an event when a notifier was dropped.
	void disable_notifier_dropped_event() {
	m_notifier_dropped_event.reset();
	}
	/// If the [`Service`] is created it disables sending an event when a notifier was identified
	/// as dead.
	void disable_notifier_dead_event() {
	m_notifier_dead_event.reset();
	}

No need to make it unnecessary complicated. This will also simplify the logic in service_builder_event.cpp

[elfenpiff]

Actually, this does not work. When this is not called, the user has not set the requirement and takes whatever QoS the service is offering. As soon as you call disable or enable with a specific event, you set this as a requirement that is ensured when opening the service.

[elBoberido]

Okay, then I do not understand what you want to achieve 😅

[elfenpiff]

I need to know if the user has called explicitly disable_notifier_dead_event. If so, I need to check if the service has disabled them, since it is a user requirement.
When the user did not explicitly call it, I use whatever the service has configured.

By default m_notifier_dead_event is set to iox::nullopt since the user did not set any dead event.

With your suggestion I have no way to distinguish the use cases, if the user explicitly disabled the event or just never set it. And I need to know it to conclude if this is a requirement or not.

[elBoberido]

Hmm, if it's three options, a new enum might indeed make sense

enum Event {
    Default, // can be on or off; maybe Preconfigured
    Custom{id},
    Disabled,
}

[elfenpiff]

We can go that way, but then we need this for every builder option in every factory for all ports and services since it is always handled like this.

I think your suggestion is not bad at all, but would require a greater refactoring. My suggestion for now:

1. Pursue the current approach and merge the PR
2. Create an issue to track this and tackle this in v0.5 or later

[elBoberido]

But you still need to adjust the builder, else there is a bug in service_builder_event.cpp.

One can call disable_notifier_created_event followed by notifier_created_event and the logic for disable_notifier_created_event will be executed

[elfenpiff]

@elBoberido I created the issue: #556

[codecov]

Codecov Report

Attention: Patch coverage is 68.58974% with 49 lines in your changes missing coverage. Please review.

Project coverage is 79.10%. Comparing base (4d60b02) to head (93734b4).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
iceoryx2/src/service/mod.rs	55.93%	26 Missing ⚠️
iceoryx2/src/service/builder/event.rs	68.51%	17 Missing ⚠️
iceoryx2/src/port/notifier.rs	80.00%	5 Missing ⚠️
iceoryx2/src/node/mod.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #552      +/-   ##
==========================================
- Coverage   79.11%   79.10%   -0.01%     
==========================================
  Files         203      203              
  Lines       25146    25293     +147     
==========================================
+ Hits        19893    20008     +115     
- Misses       5253     5285      +32     

Files with missing lines	Coverage Δ
iceoryx2/src/config.rs	77.77% <100.00%> (+0.47%)	⬆️
iceoryx2/src/service/static_config/event.rs	90.90% <100.00%> (+5.19%)	⬆️
iceoryx2/src/node/mod.rs	68.90% <66.66%> (ø)
iceoryx2/src/port/notifier.rs	89.09% <80.00%> (-1.48%)	⬇️
iceoryx2/src/service/builder/event.rs	79.76% <68.51%> (+1.39%)	⬆️
iceoryx2/src/service/mod.rs	74.03% <55.93%> (-4.31%)	⬇️

... and 3 files with indirect coverage changes