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

[core] redesign publisher api #1858

Merged
merged 21 commits into from
Dec 16, 2024
Merged

Conversation

rex-schilasky
Copy link
Contributor

@rex-schilasky rex-schilasky commented Dec 12, 2024

Description

Public API Updates

  1. Namespace Changes:

    • The old eCAL::CPublisher API has been moved to the eCAL::v5 namespace, explicitly targeting legacy eCAL version 5 applications. This ensures backward compatibility for existing systems still reliant on the older API.
  2. New API Introduction:

    • A new eCAL::CPublisher class has been introduced under the eCAL::v6 namespace (default inline namespace).
    • The new API offers a modernized, reduced interface, improving usability.

Key Differences Between v5::CPublisher and v6::CPublisher

  • Constructor Changes:

    • In v6, the Create() method has been removed. Publishers are fully initialized during construction.
    • New constructors allow optional inclusion of:
      • SDataTypeInformation for specifying data type encoding and descriptor.
      • PubEventIDCallbackT for event callback registration during initialization.
  • Destruction:

    • In v6, Destroy() has been removed. Publishers are cleaned up automatically via the destructor.
  • Send Method Overhaul:

    • v6 refines the Send method:
      • Returns a boolean (true for success, false for failure) instead of the number of bytes sent.
      • Accepts payload in three formats: raw buffer, CPayloadWriter, and std::string.
  • `Connect/Disconnect event handling:

    • old behavior:
      • connect event is fired one time when first subscriber is connecting
      • disconnect event is fired one time when last subscriber is disconnected
    • new behavior:
      • connect event is fired for every connecting subscriber
      • disconnect event is fired for every disconnecting subscriber
  • Removed Functions:

    • SetAttribute, ClearAttribute, and SetID were removed from v6 as they were experimental or seldom used.
    • The IsCreated() method is no longer necessary due to automatic resource management.
    • AddEventCallback and RemEventCallback are replaced by an optional callback parameter in the constructor.

Code Modernization

  • Resource Management:

    • Internally, both APIs now use std::shared_ptr<CPublisherImpl> for robust and automated resource management.
  • C++ Standards:

    • constexpr constants and noexcept specifications have been added for improved performance and clarity.

Impact

  • These changes enable legacy applications to remain functional while offering a streamlined, modern API for new projects.
  • Users of the new API benefit from improved usability, safer memory management, and better compliance with C++ standards.

new eCAL::CPublisher with reduced API
all tests and samples use new CPublisher
all apps, lang bindings adapted to use old v5::CPublisher
@rex-schilasky rex-schilasky added the cherry-pick-to-NONE Don't cherry-pick these changes label Dec 12, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 23. Check the log or trigger a new build to see more.

ecal/core/include/ecal/ecal_publisher.h Outdated Show resolved Hide resolved
ecal/core/include/ecal/ecal_publisher_v5.h Show resolved Hide resolved
ecal/core/include/ecal/ecal_publisher_v5.h Show resolved Hide resolved
ecal/core/include/ecal/ecal_publisher_v5.h Show resolved Hide resolved
lang/csharp/Continental.eCAL.Core/ecal_clr.h Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lang/python/core/src/ecal_clang.cpp Show resolved Hide resolved
lang/python/core/src/ecal_clang.cpp Show resolved Hide resolved
lang/python/core/src/ecal_clang.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/include/ecal/ecal_publisher.h Outdated Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/src/pubsub/ecal_publisher.cpp Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/include/ecal/ecal_publisher.h Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher.cpp Outdated Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher.cpp Outdated Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

lang/python/core/src/ecal_clang.cpp Show resolved Hide resolved
lang/python/core/src/ecal_clang.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/src/pubsub/ecal_pubgate.cpp Show resolved Hide resolved
Constructor handling (code duplicates)
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/src/pubsub/ecal_publisher.cpp Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher.cpp Show resolved Hide resolved
Copy link
Contributor

@Peguen Peguen left a comment

Choose a reason for hiding this comment

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

🫧

ecal/core/src/pubsub/ecal_publisher.cpp Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher.cpp Outdated Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher_impl.cpp Outdated Show resolved Hide resolved
ecal/core/src/pubsub/ecal_publisher_impl.cpp Show resolved Hide resolved
RemEventCallback -> RemoveEventCallback
std::move(const callback) -> = callback
check m_event_id_callback before creating callback structure
@rex-schilasky rex-schilasky merged commit a6d6f1f into master Dec 16, 2024
18 checks passed
@rex-schilasky rex-schilasky deleted the feature/redesign-publisher-api branch December 16, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants