Aggregate Entity with Talker/Listener examples

[romansavrulin]

I'm looking into writing an Entity implementation for Presonus CS18AI unit I'm working with. This is my open source initiative to do so.

I've successfully ran default library examples on the unit and ensured that the unit is enumerated in MACs AVB controller with all the zeroes when the example is running.

When I dig a little deeper into AEM entity implementation, I've discovered that Talker/Listener capability Delegates are disabled and there's no public API to set those capabilities on LocalEntity.

// Entity is listener capable
if (commonInformation.listenerCapabilities.test(ListenerCapability::Implemented))
{
    AVDECC_ASSERT(false, "TODO: AggregateEntityImpl: Handle listener capability");
    //_listenerCapabilityDelegate = std::make_unique<listener::CapabilityDelegate>(entityID);
}

// Entity is talker capable
if (commonInformation.talkerCapabilities.test(TalkerCapability::Implemented))
{
    AVDECC_ASSERT(false, "TODO: AggregateEntityImpl: Handle talker capability");
    //_talkerCapabilityDelegate = std::make_unique<talker::CapabilityDelegate>(entityID);
}

Can you give any info about what's missing in the library and how to make local entity

• Present itself as a Talker/Listener capable
• Allow to handle Talker/Listener acquisition and stream start/stop events from a remote controller

[christophe-calmejane]

Sure let's take Talker as an example.

Keep in mind that the project is split in 2 libraries:

• la_avdecc which only handled transportation (it should not hold any model)
• la_avdecc_controller which rely on the transportation library to automate API calls and holds models

In order to add the talker functionnalities to the project, you'll have to:

• handle transportation by improving _laavdecc
• create a new library la_avdecc_talker to provide a convenient representation of the Talker model and an easy to use way of communication with other devices

Working on la_avdecc

Create Talker related classes

Create a new file include/la/avdecc/internals/talkerEntity.hpp based on include/la/avdecc/internals/controllerEntity.hpp

• Define the Interface class: APIs to change the state of the entity and/or send message to the network
• Define the Delegate class: Callbacks to handle incoming events from the network
• Define the TalkerEntity class: Used to instantiate and entity only having Talker capabilities

Create Talker Capabilities class

Create a new file src/entity/talkerCapabilityDelegate.hpp based on src/entity/controllerCapabilityDelegate.hpp.

• Define the CapabilityDelegate class: The same APIs defined in Interface in include/la/avdecc/internals/talkerEntity.hpp Do the same for the .cpp file:
• Implement all the Interface methods which should be fairly easy
• Listen and handle events coming from the ProtocolInterface observer which should also be very easy as Talkers don't have to listen to many types of events (at least to begin with)

Extend AggregateEntity (can optionnaly be done later)

Extend the AggregateEntity class to be a talker:

• Inheritate from talker::Interface
• Pass a talker::Delegate to the factory
• Uncomment all lines related to Talker in the class

Working on new la_avdecc_talker

You can start by using a TalkerEntity instead of an AggregateEntity if you chose to delay extending the latter. For this library, feel free to propose something that looks like the controller one, but tailored for a Talker. I don't have any clear view on what it should look like (yet), but I guess having an easy way to create the AEM based on the provided json model would be helpful. I had plans to support this feature in the controller but couldn't find the time to do so yet. Maybe we can mutualize the code then.

Hope it will help you to get started.

[romansavrulin]

@christophe-calmejane Thanks a lot for you explanation! Looks pretty straightforward. I'll dive into this during the weekend and get back to you with some results and questions.

[romansavrulin]

@christophe-calmejane I started looking into this and have first questions.

entity:CapabilityDelegate interface class seems to have not needed dependency injection of its child classes.

The original code looks like

src/entity/entityImpl.h:519

class CapabilityDelegate
{
public:
    using UniquePointer = std::unique_ptr<CapabilityDelegate>;
    virtual ~CapabilityDelegate() = default;

    /* **** Global notifications **** */
    virtual void onControllerDelegateChanged(controller::Delegate* const delegate) noexcept = 0;
    //virtual void onListenerDelegateChanged(listener::Delegate* const delegate) noexcept = 0;
    //virtual void onTalkerDelegateChanged(talker::Delegate* const delegate) noexcept = 0;

But controller::CapabilityDelegate (specific) inherits from entity:CapabilityDelegate(generic), and generic have specific dependencies. That leads to weird circular dependency. Why do we need to bring specific delegate classes into entity:CapabilityDelegate:on<**>DelegateChanged notifications? My proposal is to convert this interface into more generic version, like

src/entity/entityImpl.h:519

/** Entity Capability delegate interface (Controller, Listener, Talker) */
class CapabilityDelegate
{
public:
    /* **** Global notifications **** */
    virtual void onDelegateChanged(entity::CapabilityDelegate* const delegate) noexcept = 0;
};

So, every specific delegate don't have to implement unrelated features and bring unrelated dependencies.

What do you think about it?

[romansavrulin]

oops, looks like I've messed up with CapabilityDelegate and entity's delegates a bit. But nevertheless, why should talker delegate be interested in listener delegate change?

[romansavrulin]

@christophe-calmejane would you be so kind to tell more about the role of different delegates in this library? I'm familiar with the delegate pattern itself, just want to get some explanation from library's perspective. What should they do to bring me closer to talker implementation.

[romansavrulin]

As for now, I'm kinda copy-pasted all controller implementation with talker one, without deep understanding how to bring this up. I've made a PR #75 for reviewing purposes, so you can track my WIP status.

[christophe-calmejane]

It's something I will have to look more deeply.

The library started as a pure Controller implementation, and slowly I started to refactor a bit so adding Talker/Listener implementation would be easy. As I never got past pure theory, maybe there is room for improvement, and only starting to implement stuff can tell.

Just keep in mind that changes to the existing base code is not advised if unecessary as the library is used by many projects and breaking it would have huge impact.

Maybe I should try to implement just a single Talker or Listener method as a PoC so that other contributors know what to do more easily. That way I would have more control over what base code has to be changed (if any).

[christophe-calmejane]

@christophe-calmejane I started looking into this and have first questions.

entity:CapabilityDelegate interface class seems to have not needed dependency injection of its child classes.

The original code looks like

src/entity/entityImpl.h:519

class CapabilityDelegate
{
public:
  using UniquePointer = std::unique_ptr<CapabilityDelegate>;
  virtual ~CapabilityDelegate() = default;

  /* **** Global notifications **** */
  virtual void onControllerDelegateChanged(controller::Delegate* const delegate) noexcept = 0;
  //virtual void onListenerDelegateChanged(listener::Delegate* const delegate) noexcept = 0;
  //virtual void onTalkerDelegateChanged(talker::Delegate* const delegate) noexcept = 0;

But controller::CapabilityDelegate (specific) inherits from entity:CapabilityDelegate(generic), and generic have specific dependencies. That leads to weird circular dependency. Why do we need to bring specific delegate classes into entity:CapabilityDelegate:on<**>DelegateChanged notifications? My proposal is to convert this interface into more generic version, like

Indeed you messed up the different Delegates, but you are right about one thing, there is no need for the base class to know about the inherited ones. But it's not for the reason you mention. During an old refactoring I forgot about those events, but are no longer events but commands. They should not belong to the CapabilityDelegate anymore. I've cleanup this a bit and I will shortly provide a new commit that fixes that! Thanks for spotting this.

[romansavrulin]

@christophe-calmejane you're welcome! It is always interesting to dive into some complex c++ stuff and try to improve one.

Regarding your proposal with talker or listener implementation: bringing one makes sense, especially in the lack of human-generated architectural documentation to the library itself (Or am I missing one?). I will also continue implementation by myself, but it can take much more time to come up with a clean solution than building one on top of provided example.

[christophe-calmejane]

I've a lot of UML on paper, but never had the time to use a proper tool and commit it. I hope I'll be able to complete this task in the near future. I'm often using the Doxygen generated UML when I need to check if something changed :)

[christophe-calmejane]

Trying to make a small implementation, I noticed it would be better (for the low level library at least) to handle Talker and Listeners the same way. I actually think we should only have 2 upper libraries, the existing controller one and a new one to handle talkers/listeners: endpoint.

I quickly made a PoC for the low level lib in order to handle such endpoints, as well as a new sample showing what the upper level library should do (I only implemented a few methods to showcase the concept). Please take a look at the branch task/endpointImplementationPoC. If it looks good enough, I suggest you start working on the upper library which would:

• Implement la::avdecc::entity::endpoint::Delegate
• Implement a state machine to easily handle IN_PROGRESS messages (automatically sending a IN_PROGRESS until the real response is sent)
• Implement a module that takes a la::avdecc::entity::model::EntityTree and automatically respond to all Descriptor Queries (this module would have to be accessible from the controller library as this is something I've wanted for a long time)
• Optionaly load the EntityTree from a dumped json model using la::avdecc::entity::model::jsonSerializer::createEntityTree
• Other stuff that we can discuss

If the PoC seems usable for you, I'll quickly complete the missing methods in the Delegate (won't take long as it's just a matter of search and replace)

[christophe-calmejane]

I just noticed something is not building on macOS (I mainly develop on windows). I'll fix the branch tomorrow.

[romansavrulin]

@christophe-calmejane Thanks for the effort! I'll check out this implementation in a couple of days

[romansavrulin]

@christophe-calmejane As for building error:

diff --git a/examples/src/CMakeLists.txt b/examples/src/CMakeLists.txt
index 19966ab..fd1736f 100644
--- a/examples/src/CMakeLists.txt
+++ b/examples/src/CMakeLists.txt
@@ -39,7 +39,7 @@ set_target_properties(SimpleEndpoint PROPERTIES FOLDER "Examples")
 target_link_libraries(SimpleEndpoint PRIVATE la_avdecc_cxx)
 copy_runtime(SimpleEndpoint la_avdecc_cxx)
 if(NOT WIN32)
-       target_link_libraries(SimpleEndpoint PRIVATE ncurses)
+       target_link_libraries(SimpleEndpoint PRIVATE ncurses pthread)
 endif()
 # Setup common options
 setup_executable_options(SimpleEndpoint)
diff --git a/include/la/avdecc/internals/protocolAemAecpdu.hpp b/include/la/avdecc/internals/protocolAemAecpdu.hpp
index bf54691..62caf81 100644
--- a/include/la/avdecc/internals/protocolAemAecpdu.hpp
+++ b/include/la/avdecc/internals/protocolAemAecpdu.hpp
@@ -69,7 +69,7 @@ public:
        LA_AVDECC_API AemAecpdu(bool const isResponse) noexcept;

        /** Destructor (for some reason we have to define it in the cpp file or clang complains about missing vtable, using = default or inline not working) */
-       virtual LA_AVDECC_API ~AemAecpdu() noexcept override;
+       virtual LA_AVDECC_API ~AemAecpdu() noexcept override {};

        // Setters
        LA_AVDECC_API void LA_AVDECC_CALL_CONVENTION setUnsolicited(bool const unsolicited) noexcept;
diff --git a/src/endStationImpl.cpp b/src/endStationImpl.cpp
index 7d84b7d..a454c58 100644
--- a/src/endStationImpl.cpp
+++ b/src/endStationImpl.cpp
@@ -29,6 +29,7 @@
 // Entities
 #include "entity/controllerEntityImpl.hpp"
 #include "entity/aggregateEntityImpl.hpp"
+#include "entity/endpointEntityImpl.hpp"

 namespace la
 {
diff --git a/src/protocol/protocolAemAecpdu.cpp b/src/protocol/protocolAemAecpdu.cpp
index 5585692..82ed6b1 100644
--- a/src/protocol/protocolAemAecpdu.cpp
+++ b/src/protocol/protocolAemAecpdu.cpp
@@ -48,7 +48,7 @@ AemAecpdu::AemAecpdu(bool const isResponse) noexcept
        Aecpdu::setAecpSpecificDataLength(HeaderLength);
 }

-AemAecpdu::~AemAecpdu() noexcept {}
+//AemAecpdu::~AemAecpdu() noexcept {}

 void LA_AVDECC_CALL_CONVENTION AemAecpdu::setUnsolicited(bool const unsolicited) noexcept
 {
@@ -154,7 +154,6 @@ void LA_AVDECC_CALL_CONVENTION AemAecpdu::deserialize(DeserializationBuffer& buf
        {
                _commandSpecificDataLength = _controlDataLength - minCDL;
        }
-
        // Check if there is more advertised data than actual bytes in the buffer (not checking earlier since we want to get as much information as possible from the packet to display a proper log message)
        auto const remainingBytes = buffer.remaining();
        if (_commandSpecificDataLength > remainingBytes)

but still failing at linking step with

[ 71%] Built target NetworkInterfacesEnumeratorCxx
[ 72%] Linking CXX executable SimpleEndpoint
[ 75%] Built target RawMessageSend
[ 77%] Built target SimpleController
[ 92%] Built target la_avdecc_controller_cxx
[ 92%] Built target la_avdecc_controller_static
make[3]: Entering directory '/build/buildroot/output/build/avdecc-v2.11.3'
make[3]: Entering directory '/build/buildroot/output/build/avdecc-v2.11.3'
Scanning dependencies of target Discovery
Scanning dependencies of target EntityDumper
make[3]: Leaving directory '/build/buildroot/output/build/avdecc-v2.11.3'
make[3]: Leaving directory '/build/buildroot/output/build/avdecc-v2.11.3'
make[3]: Entering directory '/build/buildroot/output/build/avdecc-v2.11.3'
make[3]: Entering directory '/build/buildroot/output/build/avdecc-v2.11.3'
[ 94%] Building CXX object examples/src/CMakeFiles/EntityDumper.dir/entityDumper.cpp.o
[ 94%] Building CXX object examples/src/CMakeFiles/EntityDumper.dir/utils.cpp.o
[ 94%] Building CXX object examples/src/CMakeFiles/Discovery.dir/discovery.cpp.o
[ 95%] Building CXX object examples/src/CMakeFiles/Discovery.dir/utils.cpp.o
/build/buildroot/output/host/lib/gcc/arm-buildroot-linux-gnueabi/8.3.0/../../../../arm-buildroot-linux-gnueabi/bin/ld: CMakeFiles/SimpleEndpoint.dir/simpleEndpoint.cpp.o: in function `la::avdecc::protocol::AemAecpdu::AemAecpdu(la::avdecc::protocol::AemAecpdu const&)':
/build/buildroot/output/build/avdecc-v2.11.3/include/la/avdecc/internals/protocolAemAecpdu.hpp:95: undefined reference to `vtable for la::avdecc::protocol::AemAecpdu'
collect2: error: ld returned 1 exit status
examples/src/CMakeFiles/SimpleEndpoint.dir/build.make:99: recipe for target 'examples/src/SimpleEndpoint' failed
make[3]: *** [examples/src/SimpleEndpoint] Error 1
make[3]: Leaving directory '/build/buildroot/output/build/avdecc-v2.11.3'
CMakeFiles/Makefile2:420: recipe for target 'examples/src/CMakeFiles/SimpleEndpoint.dir/all' failed
make[2]: *** [examples/src/CMakeFiles/SimpleEndpoint.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....
[ 96%] Linking CXX executable EntityDumper
Copying la_avdecc_controller_cxx shared library to EntityDumper output folder for easy debug
Copying la_avdecc_cxx shared library to EntityDumper output folder for easy debug
[ 97%] Linking CXX executable Discovery
make[3]: Leaving directory '/build/buildroot/output/build/avdecc-v2.11.3'
[ 97%] Built target EntityDumper
Copying la_avdecc_controller_cxx shared library to Discovery output folder for easy debug
Copying la_avdecc_cxx shared library to Discovery output folder for easy debug
make[3]: Leaving directory '/build/buildroot/output/build/avdecc-v2.11.3'
[ 97%] Built target Discovery
make[2]: Leaving directory '/build/buildroot/output/build/avdecc-v2.11.3'
Makefile:129: recipe for target 'all' failed
make[1]: *** [all] Error 2
make[1]: Leaving directory '/build/buildroot/output/build/avdecc-v2.11.3'
package/pkg-generic.mk:238: recipe for target '/build/buildroot/output/build/avdecc-v2.11.3/.stamp_built' failed
make: *** [/build/buildroot/output/build/avdecc-v2.11.3/.stamp_built] Error 2

Have tried both virtual destructor in .hpp and .cpp - not working yet.

Not sure what is missing, will take a look at it a bit later

[christophe-calmejane]

I fixed the compilation issues, I'll integrate my fix in dev as it's something general I should have fixed a long time ago. Don't rely too much on the branch task/endpointImplementationPoC as it's not meant to stay like this, it's just a PoC right now

[romansavrulin]

@christophe-calmejane yep, successfully built and ran SimpleEndpoint example from HEAD of task/endpointImplementationPoC for CS18AI (arm Linux platform)

[romansavrulin]

@christophe-calmejane I'll also put micro utility patches here, in order not to introduce enormous amount of branches and PRs between our repos

From 6a7f079a52e866522ce91c3ce7a2553441380b7c Mon Sep 17 00:00:00 2001
From: Roman Savrulin <romansavrulin@gmail.com>
Date: Fri, 28 Feb 2020 15:42:45 +0300
Subject: [PATCH] auto select interface if only one is available

---
 examples/src/utils.cpp | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/examples/src/utils.cpp b/examples/src/utils.cpp
index ba40ddf..dcb9408 100644
--- a/examples/src/utils.cpp
+++ b/examples/src/utils.cpp
@@ -158,6 +158,11 @@ la::avdecc::networkInterface::Interface chooseNetworkInterface()
        return {};
    }

+   if (interfaces.size() == 1)
+   {
+       return interfaces.at(0);
+   }
+
    // Let the user choose an interface
    outputText("Choose an interface:\n");
    unsigned int intNum = 1;
-- 
2.25.0

second one

From 97ef2ef4a7bb5c924c2c73a1f4543f18d7979eac Mon Sep 17 00:00:00 2001
From: Roman Savrulin <romansavrulin@gmail.com>
Date: Fri, 28 Feb 2020 16:41:15 +0300
Subject: [PATCH] remove Clion-generated directories

---
 .gitignore | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index 2602bbd..6a7b756 100644
--- a/.gitignore
+++ b/.gitignore
@@ -11,4 +11,5 @@
 /externals/3rdparty/winpcap/Include
 /externals/3rdparty/winpcap/Lib

-.idea
\ No newline at end of file
+.idea/
+cmake-build-debug/
\ No newline at end of file
-- 
2.25.0

[christophe-calmejane]

I've seen those commits in a previous PR, and might pull one or two, although it might change a bit how I proceed with my tests. Like automatically selecting the only interface, as I usually use the fact that I only have one available to detect I have an issue with my thunderbolt adapter :) I'll live with it I guess.

As for the second one, I don't like having every possible build folder in the ignore list. I've already added a generic _* pattern, I think it should cover all cases (I hope Clion allows the user to choose the output folder)

[romansavrulin]

@christophe-calmejane JetBrains offers to set RUNTIME_OUTPUT_DIRECTORY property in CMakeLists.txt file to keep things clean. Don't you mind doing so?

[raveslave]

hi, curious is anyone has completed support for configuring talker/listeners?

[christophe-calmejane]

Not to my knowledge, but it would be a nice addition. Especially now that the library has more internal features to better support entity models.