[core] create FrequencyCalculator class for sender / receiver frequency calculation. #1429
Conversation
| @@ -477,6 +474,12 @@ namespace eCAL | |||
| // increase read clock | |||
| m_clock++; | |||
|
|
|||
| // Update frequency calculation | |||
| { | |||
| std::lock_guard<std::mutex> freq_lock(m_frequency_calculator_mutex); | |||
| @@ -299,7 +293,10 @@ namespace eCAL | |||
| ecal_reg_sample_topic.pname = m_pname; | |||
| ecal_reg_sample_topic.uname = Process::GetUnitName(); | |||
| ecal_reg_sample_topic.dclock = m_clock; | |||
| ecal_reg_sample_topic.dfreq = m_freq; | |||
| { | |||
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | |||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -299,7 +293,10 @@ namespace eCAL | |||
| ecal_reg_sample_topic.pname = m_pname; | |||
| ecal_reg_sample_topic.uname = Process::GetUnitName(); | |||
| ecal_reg_sample_topic.dclock = m_clock; | |||
| ecal_reg_sample_topic.dfreq = m_freq; | |||
| { | |||
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | |||
There was a problem hiding this comment.
warning: variable 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | |
| std::lock_guard<std::mutex> const lock(m_frequency_calculator_mutex); |
| @@ -477,6 +474,12 @@ | |||
| // increase read clock | |||
| m_clock++; | |||
|
|
|||
| // Update frequency calculation | |||
| { | |||
| std::lock_guard<std::mutex> freq_lock(m_frequency_calculator_mutex); | |||
There was a problem hiding this comment.
warning: variable 'freq_lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]
| std::lock_guard<std::mutex> freq_lock(m_frequency_calculator_mutex); | |
| std::lock_guard<std::mutex> const freq_lock(m_frequency_calculator_mutex); |
| @@ -943,8 +922,15 @@ | |||
|
|
|||
| std::string CDataReader::Dump(const std::string& indent_ /* = "" */) | |||
| { | |||
| double frequency; | |||
There was a problem hiding this comment.
warning: variable 'frequency' is not initialized [cppcoreguidelines-init-variables]
ecal/core/src/readwrite/ecal_reader.cpp:26:
- #include <list>
+ #include <math.h>
+ #include <list>| double frequency; | |
| double frequency = NAN; |
| @@ -943,8 +922,15 @@ | |||
|
|
|||
| std::string CDataReader::Dump(const std::string& indent_ /* = "" */) | |||
| { | |||
| double frequency; | |||
| { | |||
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | |||
There was a problem hiding this comment.
warning: variable 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | |
| std::lock_guard<std::mutex> const lock(m_frequency_calculator_mutex); |
| @@ -408,6 +401,12 @@ namespace eCAL | |||
|
|
|||
| size_t CDataWriter::Write(CPayloadWriter& payload_, long long time_, long long id_) | |||
| { | |||
| { | |||
| // we should think about if we would like to potentially use the `time_` variable to tick with (but we would need the same base for checking incoming samples then.... | |||
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | |||
There was a problem hiding this comment.
warning: variable 'lock' of type 'std::lock_guardstd::mutex' can be declared 'const' [misc-const-correctness]
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | |
| std::lock_guard<std::mutex> const lock(m_frequency_calculator_mutex); |
| private: | ||
| float reset_factor; | ||
| time_point last_tick; | ||
| std::unique_ptr<FrequencyCalculator<T>> calculator; |
There was a problem hiding this comment.
warning: no template named 'unique_ptr' in namespace 'std' [clang-diagnostic-error]
std::unique_ptr<FrequencyCalculator<T>> calculator;
^| @@ -17,8 +17,10 @@ | |||
| * ========================= eCAL LICENSE ================================= | |||
| */ | |||
|
|
|||
| #include <vector> | |||
| #include <ecal/ecal.h> | |||
| #include <gtest/gtest.h> | |||
There was a problem hiding this comment.
warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]
#include <gtest/gtest.h>
^| double frequency; | ||
| }; | ||
|
|
||
| std::vector<MillisecondFrequencyPair> pairs = |
There was a problem hiding this comment.
warning: variable 'pairs' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
std::vector<MillisecondFrequencyPair> pairs =
^| { | ||
| // we should think about if we would like to potentially use the `time_` variable to tick with (but we would need the same base for checking incoming samples then.... | ||
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | ||
| m_frequency_calculator.addTick(std::chrono::steady_clock::now()); |
There was a problem hiding this comment.
It would technically be better to grab the current time before aquiring the mutex. But I guess in this case it doesn't matter
| double frequency; | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_frequency_calculator_mutex); | ||
| frequency = m_frequency_calculator.getFrequency(std::chrono::steady_clock::now()) * 1000000; |
There was a problem hiding this comment.
Why is this in µHz, while all other values are in mHz?
| @@ -785,6 +764,7 @@ namespace eCAL | |||
| out << indent_ << "m_topic_info.desc: " << m_topic_info.descriptor << '\n'; | |||
| out << indent_ << "m_id: " << m_id << '\n'; | |||
| out << indent_ << "m_clock: " << m_clock << '\n'; | |||
| out << indent_ << "frequency [mHz]: " << frequency << '\n'; | |||
There was a problem hiding this comment.
It says mHz here, but you multiply with 1,000,000, which should result in µHz
| } | ||
|
|
||
| private: | ||
| long long counted_elements; |
There was a problem hiding this comment.
warning: use default member initializer for 'counted_elements' [modernize-use-default-member-init]
ecal/core/src/util/frequency_calculator.h:53:
- : counted_elements(0)
+ : | long long counted_elements; | |
| long long counted_elements{0}; |
| long long counted_elements; | ||
| time_point first_tick; | ||
| time_point last_tick; | ||
| double previous_frequency; |
There was a problem hiding this comment.
warning: use default member initializer for 'previous_frequency' [modernize-use-default-member-init]
ecal/core/src/util/frequency_calculator.h:56:
- , previous_frequency(0.0)
+ , | double previous_frequency; | |
| double previous_frequency{0.0}; |
| }; | ||
|
|
||
|
|
||
| TEST(Util, FrequencyCalculator) |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| TEST(Util, FrequencyCalculator) | |
| TEST(Util /*unused*/, FrequencyCalculator /*unused*/) |
| } | ||
| } | ||
|
|
||
| TEST(Util, ResettableFrequencyCalculator) |
There was a problem hiding this comment.
warning: all parameters should be named in a function [readability-named-parameter]
| TEST(Util, ResettableFrequencyCalculator) | |
| TEST(Util /*unused*/, ResettableFrequencyCalculator /*unused*/) |
Description
The frequency calculation for reader / writer frequencies was moved from reader.cpp / writer.cpp into it's own class.
The calculator properly handles frequencies below 1Hz and is able to reset if no data is coming in.
Discussion
The calculator is not thread safe. Therefore mutexes are needed for synchronization.
The non-resettable calculator can probably be made threadsafe by moving the two members with are accessed concurrently into a struct with a size of < 8 bytes and proper memory alignment. Both functions could atomically load/store the values and a lock-free thread safety can be achieved.
The resettable calculator sees a bit harder. The unique_ptr needs to be handled and atomic unique_pointers are not available. That pointer may be created in
addTickand destroyed ingetFrequency, and I have low hopes of getting that part threadsafe easily without using locks.Related issues
Fixes #1404
Cherry-pick to
To be discussed, potentially 5.13, as it is not yet released.