eCAL 6 API Review

[KerstinKeller]

Context

We need to review API for eCAL 6.

Proposal

Misc

• eCAL inline v6 namespace: we should either use it everywhere, or maybe nowhere. Let's discuss it!
• Do we keep the old v5 pubsub / client server API for compatibility or do we just thow it out altogether. ATM it's not consistent with the old API anymore anyways.

eCAL Core API

ecal_util.h
Remove:

• EnableLoopback (set by Config) [core] Remove all instances of EnableLoopback as it can be set via … #1833
• PubShareType (set by Config) [core] Remove PubShareType / PubShareDescription (set via publisher c… #1834
• -> Remove also from Config / yaml ... [core] Completely remove the ability not to transmit type and descrip… #1885 @KerstinKeller
• PubShareDescription (set by Config) [core] Remove PubShareType / PubShareDescription (set via publisher c… #1834
• -> Remove also from Config / yaml ... [core] Completely remove the ability not to transmit type and descrip… #1885 @KerstinKeller
• SplitCombinedTopicType @KerstinKeller (deprecate only, as used still by player / recorder / others)
• CombinedTopicEncodingAndType @KerstinKeller (deprecate only, as used still by player / recorder / others)
• - Remove from Player ([play] Separate Encoding and Type in Player application. #1836)
• ShutdownCore ->remove, and ShutdownProcesses will shut down all processes @rex-schilasky

Review again: @Peguen

• ECAL_API std::string GeteCALConfigPath();
• ECAL_API std::string GeteCALUserSettingsPath();
• ECAL_API std::string GeteCALLogPath();
• ECAL_API std::string GeteCALActiveIniFile();

ecal_types.h @rex-schilasky

• SServiceId -> SServiceMethodId -> review carefully.
• ~~STopicId/SServiceID -> ID part should probably be private (e.g. not accessible by user) ~~ maybe eCAL 7@KerstinKeller

ecal_tlayer.h @KerstinKeller

• Remove: eSendMode, STLayer, [core] remove eSendMode and STLayer as configuration is now handled d… #1845

ecal_publisher.h / ecal_subscriber.h

• bool AddEventCallback(eCAL_Subscriber_Event type_, SubEventCallbackT callback_); -> Review (@rex-schilasky )
• Data members should be private
• What about SCallbackData? Should callbacks still contain id? (msg subscribers?)

ecal_registration.h

• Remove DescQualityFlags from functions
  (to review again @KerstinKeller @rex-schilasky )
• Do we still need GetTopicNames, GetServerMethodNames and GetClientMethodNames?

ecal_process.h @hannemn

• Remove ECAL_API int AddRegistrationCallback(enum eCAL_Registration_Event event_, const RegistrationCallbackT& callback_);
• Remove ECAL_API int RemRegistrationCallback(enum eCAL_Registration_Event event_);

eCAL API Redesign: @rex-schilasky

• Client:
  • New API on master
  • Implement GetServiceID ([core] get service id implementation #1875)
  • Provide/Use DataTypeInformation proto field on registration layer ([core] service client use datainfo #1876)
  • Provide new MethodCallbackT type using SServiceMethodInformation instead MethodCallbackT = std::function<int(const std::string& method_, const std::string& req_type_, const std::string& resp_type_, const std::string& request_, std::string& response_)> ([core] service client use datainfo #1876)
  • Upgrade all serialization protocol versions to v6
  • Test downward compatibility to v5
• Server:
  • New API on master
  • Implement GetServiceID ([core] get service id implementation #1875)
  • Provide/Use DataTypeInformation proto field on registration layer ([core] service client use datainfo #1876)
  • Change SServiceResponse to use SServiceMethodId instead single elements ([core] new service response struct/callback type using SServiceMethodID #1881)
  • Upgrade all serialization protocol versions to v6
  • Test downward compatibility to v5
• Publisher:
  • New API on master
  • WriterAttr -> PublisherAttr
  • Write -> Send
    • We still have 3 Send Overloads (void* & size_t, std::string, PayloadWriter) -> do we still need them all?
  • Implement event tests (v5 + v6)
  • Upgrade all serialization protocol versions to v6
  • Test downward compatibility to v5
  • Findings:
    • Do we still need GetTopicName if it is part of GetTopicId() -> or should that return only the EntityId?
• Subscriber:
  • New API on master
  • ReaderAttr -> SubscriberAttr
  • Read -> ???
  • Implement event tests (v5 + v6)
  • Upgrade all serialization protocol versions to v6
  • Test downward compatibility to v5

Deprecate / Review: @hannemn

• GetXXX (ProcessId, ProcessName, ...)
• eCAL::Process::StartProcess / StopProcess->** Move outside of eCAL?
• GetHostGroupName -> Move to internal function.

ecal_monitoring.h

• GetMonitoring -> return bool instead of int.
• [x ] Remove: SetExclFilter / SetInclFilter / SetFilter API @rex-schilasky ([core] SetExclFilter, SetInclFilter, SetFilterState removed from monitoring API #1896)
• Monitoring API structs - currently a mirror of registration information (backwards compatibility) - refactor to make it "forward" compatible? (as we would like it)
• Remove from monitoring! @rex-schilasky

        std::string           req_type;                           //<! request  type       (deprecated use req_datatype)
      std::string           req_desc;                           //<! request  descriptor (deprecated use req_datatype)
      std::string           resp_type;                          //<! response type       (deprecated use resp_datatype)
      std::string           resp_desc;                          //<! response descriptor (deprecated use resp_datatype)
  

ecal_log.h @Peguen

• Set log level via configuration
• Remove Get / Set functions of log level / filter
• Remove ECAL_API void Log(const std::string& msg_); (user should always specify level with each logging operation)

ecal_deprecate.h

• Remove eCAL 5 deprecation warnings [core] add ecal 6 deprecation warnings. Remove last deprecated function. #1844
• Add eCAL 6 deprecation warning (and deprecate SplitCombinedTopicType / old service api, ...) [core] add ecal 6 deprecation warnings. Remove last deprecated function. #1844

ecal_core.h @hannemn

• ECAL_API const char* GetVersionString();
• struct eCALVersion {int major = ECAL_VERSION_MAJOR; int minor = ECAL_VERSION_MINOR; int ECAL_VERSION_PATCH};
• eCALVersion GetVersion();
• Remove SetUnitName -> Can be set via eCAL::Initialize() (

Initialize: @hannemn

• Remove versions with command line arguments
• Add "default" version
  ECAL_API int Initialize(int argc_ = 0, char **argv_ = nullptr, const char *unit_name_ = nullptr, unsigned int components_ = Init::Default);
• Add Initialize(); version without config.
• std::string for Unitname.
• ECAL_API int SetUnitName(const char *unit_name_); -> std::string
• IsInitialized, make 2 functions, one without argument, one with component argument
• remove ECAL_CORE_COMMAND_LINE option and tclap package!

@rex-schilasky

• SEntityId.entityId should not be a string!

ecal_configuration.h @Peguen

• make some functions ecal internal, which have no "use" to users:
• ECAL_API TransportLayer::Configuration& GetTransportLayerConfiguration ();
• Move all helperfunctions to internal config header (We probably do need them, maybe we can review them)
• remove namespace Experimental` in config

ecal_callback.h / ecal_callback_cimpl.h

• Remove SServiceAttr.tcp_port_v0
• Comment Subscriber / Publisher ... Event Enums
• SClientAttr -> sid -> Registration::EntityIDT -
  check other attributes (e.g. key
• Distinguish Server / Client Attributes
  Server / Client Callbacks should contain Attributes of the other
  Might only contain ServerID / ClientID, as user can request other information via Registration method? @rex-schilasky
• Deprecate "old" EventCallbackTypes.

Make all API functions internal

• config/transport_layer.h -> same type for local / network config
• host_group_name -> shm_transport_domain @Peguen
• protocol_v0 -> remove from configuration. and remove from code. service protocol 0 removed #1846
• -> remove whole header file @rex-schilasky
• -> remove dump config....

dynamic.h

• add old signature for AddCallback.

@hannemn
ecal_XXX.h and ecal_XXX_cimpl.h

• change return type of C++ API to bool (true on success)
• align int return type of C API to corresponding C++ methods (zero on success)

Misc:

• remove ecal_def_ini.h [core] remove unused internal header file. #1886

eCAL::Configuration header files

• rename GetYamlFilePath -> GetConfigurationFilePath @Peguen

eCAL Msg API

msg/publisher.h / msg/subscriber.h @KerstinKeller

• Subscribers are still using old school callbacks. Provide new callbacks
• Provide CMessagePublisher similar to CMessageSubscriber to add Serialization specific specific types a lot easier.

canproto

• Review Publisher API, so that Send takes a builder instead of no arguments. (see also Using eCAL with capnproto #1843)

eCAL Python API

• Add missing monitoring information

eCAL C API

• Completely split C / C++ API files
  This probably requires some code duplication (E.g. some types like eCAL_Suscriber_Event are used by both C and C++ API.)
  Will also require clear naming schema ([core_c] Move implementation to lang folder. #1904)
• Review C API and remove deprecated functions.... eCAL_Sub_Receive_Alloc / eCAL_Sub_Receive_Buffer_Alloc, eCAL_Sub_SetID, eCAL_Sub_SetAttribute, and more...
• SPubEventCallbackDataC / ... need desc_length field for descriptor. Even better, we create a struct for the SDatatypeInformationC
• eCAL headers are strictly speaking no longer available when using ecal C API. We should think about what header we might want for the C API (ecal_def.h / ecal_os.h) and create C versions of them...

eCAL C# API

• Introduce new API into C# (uses all the old functions, no DatatypeInformation, ...)

No response

[DownerCase]

STopicId -> ID part should probably be private (e.g. not accessible by user)

Could we keep at least the entity_id part available? It's useful, for example, to have a registration callback save the entity ID and later use that ID to find the correct publisher/subscriber from the result of GetMonitoring. Basically, some way to uniquely identify eCAL entities is useful. (This applies to the server/clients too)

[KerstinKeller]

Yes, the id should be there, and we would support e.g. comparing for equality, but I'd like to keep the actual ID private, as it's only a pseudo random number and has no meaning to the user.

[DownerCase]

Yes, the id should be there, and we would support e.g. comparing for equality, but I'd like to keep the actual ID private, as it's only a pseudo random number and has no meaning to the user.

Sounds perfect, thanks.

[DownerCase]

Yes, the id should be there, and we would support e.g. comparing for equality, but I'd like to keep the actual ID private, as it's only a pseudo random number and has no meaning to the user.

Actually, I thought about it, and this isn't great, especially when considering non-C++ usage.

So for context I'm working on some Go bindings and applications and I store the Id selected Id to filter out the correct topic when updating the screen. The problem with your proposal to hide the value of the ID is that the filtering must be performed in C++. Additionally, it means having to keep the underlying C++ object alive for the duration I need it; which I solved for the publisher/subscribers but its needless complexity.

As you say, the absolute value of the ID doesn't inherently mean anything except "this is the unique identifier for this eCAL entity". So to me, there's no harm in being able to get the value, its not like one can do anything bad with it anyway; it just needs to be clear it is only to be effectively used as a UUID for that entity.
Exposing a serialized value (string, uint64, etc.) is of great general benefit and ease-of-use, not just for language bindings.

If you don't want to directly expose the backing mechanism for the IDs then I ask for a function to map it to a primitive type like a uint64 or string; at least then you can change the implementation should the need arrive.

Hope that's generally agreeable. 😃