Skeleton of Session Manager application #2

AlysonRog · 2025-12-18T15:01:59Z

Tâche Wrike: https://www.wrike.com/workspace.htm?acc=2548665#/folder/1747059856/tableV2?sidePanelItemId=1764894069&spaceId=1626668175&viewId=403308094
(Specs et tâches non renseignées dans Tuleap pour l'instant)

L'objectif de ce POC était d'implémenter la structure de projet tel que décrit dans cette présentation en utilisant le framework CAF et Xmake pour la chaine de compilation: https://iconeus.sharepoint.com/:p:/r/sites/Medicaldevice/_layouts/15/Doc.aspx?sourcedoc=%7BDCE286E5-C7E6-4554-8F2E-01B4E4C3CB49%7D&file=CAFArchiPresentation.pptx&action=edit&mobileredirect=true

Voir le README pour les items manquants qui sont hors scope de cette livraison.

rebase with main

SessionManager/include/SessionManager.hpp

SessionManager/src/SessionManager.cpp

SessionManager/include/SessionManager.hpp

SessionManager/xmake.lua

modules/AcquisitionModule/xmake.lua

modules/WorkflowManager/include/Workflow.hpp

modules/WorkflowManager/include/WorkflowActor.hpp

modules/WorkflowManager/include/WorkflowType.hpp

modules/WorkflowManager/src/WorkflowNeuroradiology.cpp

SessionManager/xmake.lua

modules/AcquisitionModule/xmake.lua

modules/WorkflowManager/include/Workflow.hpp

modules/EchoViewModel/include/EchoViewerActor.hpp

modules/WorkflowManager/include/WorkflowManager/WorkflowType.hpp

modules/WorkflowManager/include/WorkflowTypeIds.hpp

modules/WorkflowManager/src/WorkflowFactory.cpp

modules/WorkflowManager/include/WorkflowType.hpp

xmake.lua

modules/AcquisitionModule/include/AcquisitionModuleTypeIds.hpp

modules/EchoViewModel/include/EchoViewer.hpp

modules/WorkflowManager/include/WorkflowType.hpp

…origin

modules/EchoViewModel/include/EchoViewModel/EchoViewer.hpp

danieleTrimarchi

Great job, thanks for sharing !

Globally, I suggest to add documentation to make this more clear for newcomers.

The documentation should consist of :

appropriate naming (classes, variables)
relevant comments where necessary
documentation (sketches, ULM diagrams, online sphynx-based docs. If possible, Sphynx based docs would be compilable from both windows and linux => easier the docs is to generate, better it is :) )
unit tests showing the usage of the code

danieleTrimarchi · 2025-12-26T14:14:56Z

modules/Common/Logger/src/Logger.cpp

+void initLoggerImpl(const LoggerConfig& cfg)
+{
+	std::error_code ec;
+	std::filesystem::create_directories(cfg.log_dir, ec);


si this not redundant wrt the code @l.71 ?

Code was a work in progress and not yet finished. And indeed there was an issue with dual directory creation ^^'. It is fixed now with the new implementation.

danieleTrimarchi · 2025-12-26T14:16:47Z

modules/Common/Logger/include/Logger/LoggerConfig.hpp

shall this config (or part of it) not rather be a runtime-loadeable file ?
ie: enable_console_output, Rotation settings,

I intend to add a configuration file with a dedicated parser that is able to populate this LoggerConfig object. And you are right this is an important feature to have on a system to enable debug logs for investigation.
coming soon :3

danieleTrimarchi · 2025-12-26T15:17:29Z

modules/AcquisitionModule/src/AcquisitionModuleActor.cpp

+{
+	return {[self](caf::get_atom)
+	        {
+		        return self->make_observable().iota(1).take(5).to_stream(


Please add some comments on this function. While the role of the static sourceFun is clear, its implementation is not.
What is the chain : make_observable().iota(1).take(5) ?
where do you define that "int-flow" actually returns ints ?

edit => please add a reference to your source : https://actor-framework.readthedocs.io/en/latest/core/DataFlows.html

This function is a dummy observer just to test the stream pattern from CAF. This is going to change soon with the addition of a real acquisition and processing stream.

danieleTrimarchi · 2025-12-26T15:20:56Z

modules/AcquisitionModule/src/AcquisitionModuleActor.cpp

+ * @brief Consumes a stream from a source actor and forwards each item to one or more
+ * destination actors.
+ *
+ * @tparam Dests Variadic template parameter pack representing the types of destination


variadic template ? I see a std::vectorcaf::actor

You are right this was from a previous version. Removed

danieleTrimarchi · 2025-12-26T15:41:05Z

modules/AcquisitionModule/src/AcquisitionModuleActor.cpp

+	        [=](caf::stream s)
+	        {
+		        // Turn the stream handle into an observable, then consume it.
+		        self->observe_as<int>(s, 50u, 10u)


I do not find observe_as in the CAF documentation ( I do find observe_on though )
how reliable is this documentation ? did you build the doc locally to work with the latest version ?

https://actor-framework.readthedocs.io/en/latest/core/DataFlows.html

You can find documentation about flow/stream with CAF here: https://www.interance.io/learning/cpp/guide/flows-part-1 (this website is a little bit better documented)

danieleTrimarchi · 2025-12-29T09:51:20Z

SessionManager/src/SessionManager.cpp

+SessionManager::SessionManager(caf::actor_system& system)
+{
+	// Spawn acquisition view model actor.
+	// STATEFUL to keep the state of the display.


if this is the actor in charge for the GUI/Display, wont it be more explicit to name it GUIActor ?

No this is the actor in charge of keeping the display state and logic. Another actor may be needed for the GUI part. I am not sure yet...

But the comment was misleading and is here because I intended to call the viewer AcquisitionView. I have renamed it into echoViewer ^^

danieleTrimarchi · 2025-12-29T10:06:18Z

modules/WorkflowManager/include/WorkflowManager/Workflow.hpp

+ * @brief Base class for the workflows. This class handles the specialization behavior
+ * between the different types of workflows (Neonate, Neuroradiology etc). It holds a
+ * state machine that guides the session through the different workflow steps.
+ */


the state machine is not yet there I guess..? : )

Besides, the role of the workflow and its interactions with the other components (echo_viewer_actor, acquisitionModuleActor) is not very clear to me.

I guess :

the workflow interacts with the GUI to guide the user trough the initial configuration steps => the GUI is modified by the workflow

the workflow interacts with the Acquisition to setup the sequences, define the accepted probes

Would you detail this point in some additional comments

This is normal to be puzzled. Some propositions about the design still need to mature.
For now I intend to have:

the view-model (echoViewer for now here) that communicates with the GUI and may communicate with the workflowManager depending on the user interaction (start of acquisition, start of post-processing, retake etc). And the workflowManager may notify to the view_model of a new state in the workflow that may affect the GUI (depending on the view-model).

the workflow is the only one to send messages to the acquisitionModule. and the AcquisitionModule flows down acquired data to the actors that the WorkflowManager gave to the AcquisitionModule.

This is all going to be documented in the Detailed Architecture for which an overview will be presented in a TDR (end of January or beginning of February). This Detailed Architecture document shall be the ground truth for the design and be updated accordingly to the software updates. Duplicating the information in comments duplicates the information and increases the risk of forgotten and outdated documentation.

danieleTrimarchi · 2025-12-29T10:24:26Z

modules/AcquisitionModule/src/AcquisitionModuleActor.cpp

+		    // Handle acquisition request (one day it will be a call to Moduleus
+		    // facade)
+		    AcquisitionModule acqModule;
+		    acqModule.acquisitionRequest(parameterValue);


Clarify by means of a comment when and how this lambda is called.

The question to be answered is : what is the interaction between the acquisitionRequest and the workflow ?

The comment should point to : workflow_actor::behavior_type workflow_actor_state::make_behavior()

Same answer that above. the design still needs to mature and interactions between actors would be better documented in a single place which would be the Detailed Design document.

danieleTrimarchi · 2025-12-29T10:26:02Z

modules/WorkflowManager/tests/unit_tests/WorkflowActorTest.cpp

+	return ([](caf::get_atom) { std::cout << "" << std::endl; });
+}
+
+TEST("a simple test")


Please implement some tests : these are a great way to explicit the intended use of the code

CAF has not documented the way to test unitarily CAF actors through messages yet... Besides AcquisitionModuleActor is still a dummy actor. Unit test will be necessary when it will start to have real behavior.

danieleTrimarchi · 2025-12-29T10:26:41Z

modules/AcquisitionModule/tests/unit_tests/AcquisitionModuleActorTest.cpp

+
+TEST("a simple test")
+{
+	check_eq(1, 1);


Please implement some tests : these are a great way to explicit the intended use of the code

CAF has not documented the way to test unitarily CAF actors through messages yet... Besides AcquisitionModuleActor is still a dummy actor. Unit test will be necessary when it will start to have real behavior.

charlesseizilles · 2026-01-05T15:15:53Z

SessionManager/src/main.cpp

+	catch (std::exception& e)
+	{
+		std::cerr << "unhandled exception: " << e.what() << "\n";
+		medlog::shutdown();


Please prefer using RRID (aka RAII...) for resource lifetime management. I recommend guideline support library's gsl::finally for that.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#e19-use-a-final_action-object-to-express-cleanup-if-no-suitable-resource-handle-is-available

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r1-manage-resources-automatically-using-resource-handles-and-raii-resource-acquisition-is-initialization

Microsoft's implementation is available in xmake-repo

https://github.com/xmake-io/xmake-repo/blob/master/packages/m/microsoft-gsl/xmake.lua

https://github.com/microsoft/GSL

I have done some modification to have a Logger class that shall be instantiated to initialize the logging resources. Its destructor performs the spdlog shutdown and the reset of global variables. It seems to me that the finally is now unnecessary as resources are properly managed now and we are sure that Logger's destructor is called with the stack unwinding. Correct me if I am wrong.

modules/Common/Logger/src/Logger.cpp

modules/Common/Logger/include/Logger/LoggerLevel.hpp

modules/Common/Logger/src/LoggerConfig.cpp

modules/Common/Logger/include/Logger/LoggerLevel.hpp

modules/Common/Logger/src/LoggerConfig.cpp

modules/WorkflowManager/include/WorkflowManager/WorkflowType.hpp

SessionManager/src/main.cpp

AlysonRog added 4 commits December 17, 2025 13:09

First implementation of Session Manager

e918d94

Update README

0428c5a

Minor improvements

39dcc86

Merge pull request #1 from Iconeus/main

b6fabb6

rebase with main

AlysonRog requested review from Thibaud-Fidel, charlesseizilles, danieleTrimarchi and jicoselene December 18, 2025 15:01

AlysonRog self-assigned this Dec 18, 2025

charlesseizilles requested changes Dec 18, 2025

View reviewed changes

AlysonRog added 11 commits December 22, 2025 15:04

Fix after code review

5302f1f

Minor code review changes

d9b3c37

add unit test targets and Jenkinsfile build + UT run

b1da70c

Add label of node to run jenkins job

04617aa

Setup CI

05bafb6

Setup CI

46635c9

Setup CI

91a7e9a

Setup CI

40378cc

Rename include guards

45cef98

Remove useless inline keyword and add missing explicit

75d4baf

Setup CI

8938c6f

charlesseizilles requested changes Dec 22, 2025

View reviewed changes

AlysonRog added 7 commits December 22, 2025 17:57

Add Neurosurgery dummy workflow

42f72c4

Rename workflows

a181bef

Setup CI

224e84d

Setup CI

ad0fe17

Update include guards and xmake.lua

3aa714c

replace module names by snake_case

56594b8

Fix include guards

612858f

charlesseizilles requested changes Dec 22, 2025

View reviewed changes

modules/AcquisitionModule/include/AcquisitionModuleTypeIds.hpp Outdated Show resolved Hide resolved

modules/EchoViewModel/include/EchoViewer.hpp Outdated Show resolved Hide resolved

modules/WorkflowManager/include/WorkflowType.hpp Outdated Show resolved Hide resolved

AlysonRog added 2 commits December 23, 2025 09:50

Fix code review issues

4cbe6e7

Add Module folder under include for better identification of headers …

e61c423

…origin

charlesseizilles requested changes Dec 23, 2025

View reviewed changes

modules/EchoViewModel/include/EchoViewModel/EchoViewer.hpp Outdated Show resolved Hide resolved

AlysonRog added 4 commits December 23, 2025 16:46

Add xmake packages into the includePath

84c477d

Fix namespace

fede0b4

Add CAF configuration file

eb19c2d

Add custom logger. Work In Progress

a6d54b7

danieleTrimarchi reviewed Dec 29, 2025

View reviewed changes

AlysonRog added 10 commits December 29, 2025 16:34

Update logger

c2d28c9

Add support of filename and line

e807f4a

Truncate the filename

e839927

Remove global state variables for logger

5884738

Set future and promises for logcheck

7507f74

Replace assertions by exceptions and improve unit test

78d66fd

Add stress tests

87e785f

Add missing rpath

397406f

Use stringview in unit test

f3fc1c1

Reduce test execution time

cfab7f5

charlesseizilles requested changes Jan 5, 2026

View reviewed changes

Refactor Logger

3cac02b

charlesseizilles requested changes Jan 6, 2026

View reviewed changes

AlysonRog added 3 commits January 6, 2026 17:17

Add logger class for RAII resource management

776e930

Add documentation

ca80fb0

Remove redundant spdlog call

17153fb

charlesseizilles requested changes Jan 6, 2026

View reviewed changes

Fix dumb mistakes

e0f2e4c

Skeleton of Session Manager application #2

Are you sure you want to change the base?

Skeleton of Session Manager application #2

Uh oh!

Conversation

AlysonRog commented Dec 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danieleTrimarchi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlysonRog Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlysonRog Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

danieleTrimarchi left a comment •

edited

Loading

AlysonRog Dec 31, 2025 •

edited

Loading

AlysonRog Dec 31, 2025 •

edited

Loading