BufferManager: add mutex, configuration from file, empty constructor workflow et al #65

Nicogene · 2021-02-05T16:42:40Z

The ChannelInfo has been changed from a struct of two elements to a
std::pair, in order to be easily handled with the JSON file conf.
All the flags, options of the BufferManager have been put in the BufferConfig
The empty constructor is allowed now, for this we added all the functions
needed for configuring a default BufferManager(It fixes Support use of the default argument-less constructor for BufferManager class #60)
The mutex has been added in saveToFile(partially address Understand how to ensure mutual exclusion of the buffer #9)
Now the periodicSave, saves only the variables that go above the
data_treshold specified
Fixes the bug of not saving when the threshold > n_samples(fixes Enforce relative condition between threshold (the max number of samples before) and number of samples #61 )
It has been added the configuration from JSON file using this library with relative example (it fixes Investigation file-based configuration of the buffer manager #39)
It add the possibility to save BufferConfig to json file(it fixes Add support to save the configuration #68)
It add nlhomann_json v3.9.0 as dependency via FetchContent( it fixes json parser: use fetch content #66)
Here is an example of JSON file for configuring the BufferManager:

{
    "filename": "buffer_manager_test_conf_file",
    "n_samples": 20,
    "save_period": 1.0,
    "data_threshold": 10,
    "auto_save": true,
    "save_periodically": true,
    "channels": [
        ["one",[1,1]],
        ["two",[1,1]]
    ]
  }

For now we used the json library just importing the json.hpp file, this has to be in the future to an "extern" folder or better with the cmake FetchContent mechanism.

Please review code.

cc @traversaro @S-Dafarra @GiulioRomualdi

- The ChannelInfo has been changed from a struct of two elements to a std::pair, in order to be easily handled with the json file conf. - All the flags, options of the BufferManager have been put in the BufferConfig - The empty constructor is allowed now, for this we added all the functions needed for configuring a default BufferManager(It fixes #60) - The mutex has been added in saveToFile(partially address #9 - Now the periodicSave, saves only the variables that go above the data_treshold specified

traversaro · 2021-02-05T16:59:20Z

src/libYARP_telemetry/src/yarp/telemetry/BufferManager.h

+        if (ok && _bufferConfig.save_periodically) {
+            ok = ok && enablePeriodicSave(_bufferConfig.save_period);
+        }
+        // TODO ROLL BACK IN CASE OF FAILURE


If you do not plan to address these, please open issues for any open points. TODO in the code get forgotten quite easily, better to open issues to track then.

Issue #72 opened

traversaro · 2021-02-05T17:04:39Z

src/libYARP_telemetry/src/yarp/telemetry/BufferManager.h

+    bool configureFromFile(const std::string& config_filename) {
+        // read a JSON file
+        std::ifstream input_stream(config_filename);
+        nlohmann::json jason_file;
+        input_stream >> jason_file;
+        return configure(jason_file.get<yarp::telemetry::BufferConfig>());
+    }


Can't we have this as a free function instead, something like bool configureFromJSONFile(BufferManager& manager, const std::string& config_filename) ?
This would avoid needles coupling of the specific configuration method with the BufferManager class.

This has been addressed in b5b431e.

Instead of BufferManager I put as I/O BufferConfig, in order to avoid circular dependencies

Nicogene · 2021-02-05T17:18:40Z

Before merging this maybe we should address #66 that seems straightforward and in this way, we avoid having so many additions

traversaro · 2021-02-08T09:38:32Z

CMakeLists.txt

@@ -62,6 +62,23 @@ find_package(matioCpp REQUIRED)
 find_package(Boost REQUIRED)
 find_package(Threads REQUIRED)

+option(YARP_TELEMETRY_USES_SYSTEM_nlohmann_json OFF)
+if(YARP_TELEMETRY_USES_SYSTEM_nlohmann_json)
+  find_package(nlohmann_json 3.9.0 REQUIRED)


Do you really require 3.9? Ubuntu 20.04 ships with 3.7 in apt (https://repology.org/project/nlohmann-json/versions), I would try to at least enable it, if then it does not work we can fix it to the minimum working version.

We need 3.9.0 for this macro. But we can write the functions by hand.

If we actually need 3.9 then it is ok for me!

Nicogene · 2021-02-08T09:47:16Z

Looking https://github.com/robotology-playground/yarp-telemetry/pull/65/checks?check_run_id=1853547617 seems that someone is putting BUILD_TESTING to true

Nicogene · 2021-02-08T09:57:59Z

Here is why nlohmann/json#2513, we need 3.9.2(that is unreleased)

traversaro · 2021-02-08T10:08:05Z

Here is why nlohmann/json#2513, we need 3.9.2(that is unreleased)

FetchContent can also use a specific commit, and then we can switch to 3.9.2 once is released.

It fixes #66

…truct We have to move these in different header It fixes #39

Nicogene · 2021-02-08T10:09:41Z

FetchContent can also use a specific commit, and then we can switch to 3.9.2 once is released.

I put develop branch

In this way we can have multiple parsers implementation and keep the parser library as private dependency

Nicogene added 3 commits February 5, 2021 17:12

Buffer-BufferManager: add resize

7c85caa

examples: change examples accordingly

0d73cc1

Nicogene added the enhancement New feature or request label Feb 5, 2021

Nicogene requested review from traversaro and S-Dafarra February 5, 2021 16:42

Nicogene assigned AlexAntn and Nicogene Feb 5, 2021

This was referenced Feb 5, 2021

Enforce relative condition between threshold (the max number of samples before) and number of samples #61

Closed

Investigation file-based configuration of the buffer manager #39

Closed

json parser: use fetch content #66

Closed

traversaro reviewed Feb 5, 2021

View reviewed changes

Nicogene mentioned this pull request Feb 5, 2021

Handle the roll back in case failure of configure() #67

Open

Nicogene force-pushed the feat/mutexFileConfEmptyCons branch from 5ae9a26 to eaa72fd Compare February 8, 2021 09:36

traversaro reviewed Feb 8, 2021

View reviewed changes

Nicogene added 3 commits February 8, 2021 11:08

YARP_telemety: add nlohmann_json parser dependency

486b96d

It fixes #66

BufferManager: add conversion from and to json for the BufferConfig s…

b0624cd

…truct We have to move these in different header It fixes #39

examples: add examples for configuration from and to json file

857945e

Nicogene force-pushed the feat/mutexFileConfEmptyCons branch from eaa72fd to 857945e Compare February 8, 2021 10:08

BufferConfig: refactored in a separated header/src

b5b431e

In this way we can have multiple parsers implementation and keep the parser library as private dependency

Nicogene requested a review from traversaro February 9, 2021 11:10

traversaro approved these changes Feb 9, 2021

View reviewed changes

Nicogene merged commit c6b9222 into master Feb 9, 2021

Nicogene deleted the feat/mutexFileConfEmptyCons branch February 9, 2021 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BufferManager: add mutex, configuration from file, empty constructor workflow et al #65

BufferManager: add mutex, configuration from file, empty constructor workflow et al #65

Nicogene commented Feb 5, 2021 •

edited

Loading

traversaro Feb 5, 2021

Nicogene Feb 9, 2021

traversaro Feb 5, 2021

Nicogene Feb 9, 2021

Nicogene commented Feb 5, 2021

traversaro Feb 8, 2021

Nicogene Feb 8, 2021

traversaro Feb 8, 2021

Nicogene commented Feb 8, 2021

Nicogene commented Feb 8, 2021 •

edited

Loading

traversaro commented Feb 8, 2021

Nicogene commented Feb 8, 2021

BufferManager: add mutex, configuration from file, empty constructor workflow et al #65

BufferManager: add mutex, configuration from file, empty constructor workflow et al #65

Conversation

Nicogene commented Feb 5, 2021 • edited Loading

traversaro Feb 5, 2021

Choose a reason for hiding this comment

Nicogene Feb 9, 2021

Choose a reason for hiding this comment

traversaro Feb 5, 2021

Choose a reason for hiding this comment

Nicogene Feb 9, 2021

Choose a reason for hiding this comment

Nicogene commented Feb 5, 2021

traversaro Feb 8, 2021

Choose a reason for hiding this comment

Nicogene Feb 8, 2021

Choose a reason for hiding this comment

traversaro Feb 8, 2021

Choose a reason for hiding this comment

Nicogene commented Feb 8, 2021

Nicogene commented Feb 8, 2021 • edited Loading

traversaro commented Feb 8, 2021

Nicogene commented Feb 8, 2021

Nicogene commented Feb 5, 2021 •

edited

Loading

Nicogene commented Feb 8, 2021 •

edited

Loading