Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[measurement] HDF5 v6 - Store channel ID in measurement #1375

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[measurement] HDF5 v6 - Store channel ID in measurement #1375

wants to merge 4 commits into from

Conversation

KerstinKeller
Copy link
Contributor

Description

Related issues

This is looseley related to #1076, and also to #1292

Cherry-pick to

  • master-v5 (master for eCAL 5 feature backporting) ??? (To be discussed - probably not. Maybe we can add the v6 reader to master-v5, however this requires a separate commit.)

Docs???

github-actions[bot]
github-actions bot reviewed Feb 16, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 55. Check the log or trigger a new build to see more.

app/meas_cutter/src/measurement_exporter.cpp
@@ -76,12 +76,12 @@ void MeasurementExporter::setData(eCALMeasCutterUtils::Timestamp timestamp, cons
const auto sender_timestamp = (iter != meta_data.end()) ? iter->second.sender_timestamp : static_cast<eCALMeasCutterUtils::Timestamp>(0);

iter = meta_data.find(eCALMeasCutterUtils::MetaDatumKey::SENDER_ID);
const auto sender_id = (iter != meta_data.end()) ? iter->second.sender_id : static_cast<uint64_t>(0);
const auto sender_id = (iter != meta_data.end()) ? iter->second.sender_id : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

  const auto sender_id = (iter != meta_data.end()) ? iter->second.sender_id : 0;
                                                                  ^

app/meas_cutter/src/measurement_exporter.cpp

iter = meta_data.find(eCALMeasCutterUtils::MetaDatumKey::SENDER_CLOCK);
const auto sender_clock = (iter != meta_data.end()) ? iter->second.sender_clock : static_cast<uint64_t>(0);
const auto sender_clock = (iter != meta_data.end()) ? iter->second.sender_clock : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

  const auto sender_clock = (iter != meta_data.end()) ? iter->second.sender_clock : 0;
                                                                     ^

app/meas_cutter/src/utils.h

std::array<char,64> __union_size;
std::array<char,64> __union_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: declaration uses identifier '__union_size', which is a reserved identifier [bugprone-reserved-identifier]

Suggested change
std::array<char,64> __union_size;
std::array<char,64> _union_size;

contrib/ecalhdf5/src/eh5_meas.cpp
auto escaped_channels = hdf_meas_impl_->GetChannels();
for (const auto& escaped_channel : escaped_channels)
{
ret_val.emplace(eCAL::eh5::SChannel{ GetUnescapedString(escaped_channel.name), escaped_channel.id });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unnecessary temporary object created while calling emplace [modernize-use-emplace]

Suggested change
ret_val.emplace(eCAL::eh5::SChannel{ GetUnescapedString(escaped_channel.name), escaped_channel.id });
ret_val.emplace( GetUnescapedString(escaped_channel.name), escaped_channel.id );

contrib/ecalhdf5/src/eh5_meas.cpp
for (const auto& escaped_channel : escaped_channels)
{
if (GetUnescapedString(escaped_channel.name) == channel_name)
ret_val.emplace(eCAL::eh5::SChannel{ channel_name, escaped_channel.id });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unnecessary temporary object created while calling emplace [modernize-use-emplace]

Suggested change
ret_val.emplace(eCAL::eh5::SChannel{ channel_name, escaped_channel.id });
ret_val.emplace( channel_name, escaped_channel.id );

contrib/ecalhdf5/src/eh5_meas_file_writer_v6.cpp
if (!IsOk())
return false;

hsize_t hsSize = static_cast<hsize_t>(size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'hsSize' of type 'hsize_t' (aka 'unsigned long long') can be declared 'const' [misc-const-correctness]

Suggested change
hsize_t hsSize = static_cast<hsize_t>(size);
hsize_t const hsSize = static_cast<hsize_t>(size);

contrib/ecalhdf5/src/eh5_meas_file_writer_v6.cpp
auto dataSet = H5Dcreate(file_id_, std::to_string(entries_counter_).c_str(), H5T_NATIVE_UCHAR, dataSpace, H5P_DEFAULT, dsProperty, H5P_DEFAULT);

// Write buffer to dataset
herr_t writeStatus = H5Dwrite(dataSet, H5T_NATIVE_UCHAR, H5S_ALL, H5S_ALL, H5P_DEFAULT, data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'writeStatus' of type 'herr_t' (aka 'int') can be declared 'const' [misc-const-correctness]

Suggested change
herr_t writeStatus = H5Dwrite(dataSet, H5T_NATIVE_UCHAR, H5S_ALL, H5S_ALL, H5P_DEFAULT, data);
herr_t const writeStatus = H5Dwrite(dataSet, H5T_NATIVE_UCHAR, H5S_ALL, H5S_ALL, H5P_DEFAULT, data);

contrib/ecalhdf5/src/eh5_meas_file_writer_v6.cpp Outdated
H5Pclose(dsProperty);
H5Sclose(dataSpace);

channels_[channel_name][id].Entries.emplace_back(SEntryInfo(rcv_timestamp, static_cast<long long>(entries_counter_), clock, snd_timestamp, id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unnecessary temporary object created while calling emplace_back [modernize-use-emplace]

Suggested change
channels_[channel_name][id].Entries.emplace_back(SEntryInfo(rcv_timestamp, static_cast<long long>(entries_counter_), clock, snd_timestamp, id));
channels_[channel_name][id].Entries.emplace_back(rcv_timestamp, static_cast<long long>(entries_counter_), clock, snd_timestamp, id);

contrib/ecalhdf5/src/eh5_meas_file_writer_v6.cpp
filePath += ".hdf5";

// create file access property
hid_t fileAccessPropery = H5Pcreate(H5P_FILE_ACCESS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'fileAccessPropery' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t fileAccessPropery = H5Pcreate(H5P_FILE_ACCESS);
hid_t const fileAccessPropery = H5Pcreate(H5P_FILE_ACCESS);

contrib/ecalhdf5/src/eh5_meas_file_writer_v6.cpp
// create file access property
hid_t fileAccessPropery = H5Pcreate(H5P_FILE_ACCESS);
// create file create property
hid_t fileCreateProperty = H5Pcreate(H5P_FILE_CREATE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'fileCreateProperty' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t fileCreateProperty = H5Pcreate(H5P_FILE_CREATE);
hid_t const fileCreateProperty = H5Pcreate(H5P_FILE_CREATE);

@KerstinKeller KerstinKeller marked this pull request as draft February 28, 2024 08:15
@KerstinKeller
Copy link
Contributor Author

Will be reimplemented.

@KerstinKeller KerstinKeller reopened this Apr 22, 2024
github-actions[bot]
github-actions bot reviewed Apr 22, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 20 out of 35. Check the log or trigger a new build to see more.

contrib/ecalhdf5/src/eh5_meas_file_writer_v6.cpp
bool eCAL::eh5::HDF5MeasFileWriterV6::EntryFitsTheFile(const hsize_t& size) const
{
hsize_t fileSize = 0;
bool status = GetFileSize(fileSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'status' of type 'bool' can be declared 'const' [misc-const-correctness]

Suggested change
bool status = GetFileSize(fileSize);
bool const status = GetFileSize(fileSize);

contrib/ecalhdf5/src/eh5_meas_file_writer_v6.cpp
// const size_t dataSetsSize = entries.size();
// if (dataSetsSize == 0) return false;

std::string hex_id = printHex(channelId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'hex_id' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change
std::string hex_id = printHex(channelId);
std::string const hex_id = printHex(channelId);

contrib/ecalhdf5/src/hdf5_helper.cpp
bool CreateStringEntryInRoot(hid_t root, const std::string& url, const std::string& dataset_content)
{
// create scalar dataset
hid_t scalar_dataspace = H5Screate(H5S_SCALAR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'scalar_dataspace' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t scalar_dataspace = H5Screate(H5S_SCALAR);
hid_t const scalar_dataspace = H5Screate(H5S_SCALAR);

contrib/ecalhdf5/src/hdf5_helper.cpp
// create scalar dataset
hid_t scalar_dataspace = H5Screate(H5S_SCALAR);
// create new string data type
hid_t string_data_type = H5Tcopy(H5T_C_S1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'string_data_type' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t string_data_type = H5Tcopy(H5T_C_S1);
hid_t const string_data_type = H5Tcopy(H5T_C_S1);

contrib/ecalhdf5/src/hdf5_helper.cpp
//H5Pset_create_intermediate_group(ds_property, 1);

// create attribute
hid_t data_set = H5Dcreate(root, url.c_str(), string_data_type, scalar_dataspace, H5P_DEFAULT, ds_property, H5P_DEFAULT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'data_set' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t data_set = H5Dcreate(root, url.c_str(), string_data_type, scalar_dataspace, H5P_DEFAULT, ds_property, H5P_DEFAULT);
hid_t const data_set = H5Dcreate(root, url.c_str(), string_data_type, scalar_dataspace, H5P_DEFAULT, ds_property, H5P_DEFAULT);

contrib/ecalhdf5/src/hdf5_helper.cpp
hid_t scalarDataset = H5Screate(H5S_SCALAR);

// create new string data type
hid_t stringDataType = H5Tcopy(H5T_C_S1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'stringDataType' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t stringDataType = H5Tcopy(H5T_C_S1);
hid_t const stringDataType = H5Tcopy(H5T_C_S1);

contrib/ecalhdf5/src/hdf5_helper.cpp
H5Tset_size(stringDataType, value.length());

// create attribute
hid_t attribute = H5Acreate(id, name.c_str(), stringDataType, scalarDataset, H5P_DEFAULT, H5P_DEFAULT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'attribute' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t attribute = H5Acreate(id, name.c_str(), stringDataType, scalarDataset, H5P_DEFAULT, H5P_DEFAULT);
hid_t const attribute = H5Acreate(id, name.c_str(), stringDataType, scalarDataset, H5P_DEFAULT, H5P_DEFAULT);

contrib/ecalhdf5/src/hdf5_helper.cpp
if (attribute < 0) return false;

// write attribute value to attribute
herr_t writeStatus = H5Awrite(attribute, stringDataType, value.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'writeStatus' of type 'herr_t' (aka 'int') can be declared 'const' [misc-const-correctness]

Suggested change
herr_t writeStatus = H5Awrite(attribute, stringDataType, value.c_str());
herr_t const writeStatus = H5Awrite(attribute, stringDataType, value.c_str());

contrib/ecalhdf5/src/hdf5_helper.cpp
if (H5Aexists(id, name.c_str()) != 0)
{
// open attribute by name, getting the attribute index
hid_t attr_id = H5Aopen_name(id, name.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'attr_id' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t attr_id = H5Aopen_name(id, name.c_str());
hid_t const attr_id = H5Aopen_name(id, name.c_str());

contrib/ecalhdf5/src/hdf5_helper.cpp
if (attr_id <= 0) return false;

// get attribute type
hid_t attr_type = H5Aget_type(attr_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'attr_type' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t attr_type = H5Aget_type(attr_id);
hid_t const attr_type = H5Aget_type(attr_id);

github-actions[bot]
github-actions bot reviewed May 15, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

contrib/ecalhdf5/src/hdf5_helper.cpp
// get attribute type
hid_t attr_type = H5Aget_type(attr_id);
// get type class based on attribute type
H5T_class_t type_class = H5Tget_class(attr_type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'type_class' of type 'H5T_class_t' can be declared 'const' [misc-const-correctness]

Suggested change
H5T_class_t type_class = H5Tget_class(attr_type);
H5T_class_t const type_class = H5Tget_class(attr_type);

contrib/ecalhdf5/src/hdf5_helper.cpp
// if attribute class is string
if (type_class == H5T_STRING)
{
hid_t attr_type_mem = H5Tget_native_type(attr_type, H5T_DIR_ASCEND);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'attr_type_mem' of type 'hid_t' (aka 'long') can be declared 'const' [misc-const-correctness]

Suggested change
hid_t attr_type_mem = H5Tget_native_type(attr_type, H5T_DIR_ASCEND);
hid_t const attr_type_mem = H5Tget_native_type(attr_type, H5T_DIR_ASCEND);

contrib/ecalhdf5/src/hdf5_helper.cpp
// create buffer to store the value of the attribute
std::vector<char> content_buffer(attr_size);
// get attribute value
ret_val = (H5Aread(attr_id, attr_type_mem, &content_buffer[0]) >= 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]

Suggested change
ret_val = (H5Aread(attr_id, attr_type_mem, &content_buffer[0]) >= 0);
ret_val = (H5Aread(attr_id, attr_type_mem, content_buffer.data()) >= 0);

contrib/ecalhdf5/src/hdf5_helper.cpp
ret_val = (H5Aread(attr_id, attr_type_mem, &content_buffer[0]) >= 0);

// convert value to std string
value = std::string(&content_buffer[0], attr_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer]

Suggested change
value = std::string(&content_buffer[0], attr_size);
value = std::string(content_buffer.data(), attr_size);

contrib/ecalhdf5/src/hdf5_helper.cpp
hid_t OpenOrCreateGroup(hid_t root, const std::string& name)
{
auto exists = H5Lexists(root, name.c_str(), H5P_DEFAULT);
hid_t group;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'group' is not initialized [cppcoreguidelines-init-variables]

Suggested change
hid_t group;
hid_t group = 0;

contrib/measurement/base/include/ecal/measurement/base/types.h
std::string descriptor; //!< descriptor information of the datatype (necessary for reflection)
std::string name = ""; //!< name of the datatype
std::string encoding = ""; //!< encoding of the datatype (e.g. protobuf, flatbuffers, capnproto)
std::string descriptor = ""; //!< descriptor information of the datatype (necessary for reflection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no type named 'string' in namespace 'std' [clang-diagnostic-error]

          std::string descriptor = "";    //!< descriptor information of the datatype (necessary for reflection)
               ^

contrib/measurement/base/include/ecal/measurement/base/types.h
{
using id_t = std::int64_t;

std::string name = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no type named 'string' in namespace 'std' [clang-diagnostic-error]

          std::string name = "";
               ^

contrib/measurement/base/include/ecal/measurement/base/types.h
id_t id = 0;

[[deprecated("Please construct a Channel object explicitly.")]]
Channel(const std::string name_) : name(name_) {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no type named 'string' in namespace 'std' [clang-diagnostic-error]

          Channel(const std::string name_) : name(name_) {};
                             ^

contrib/measurement/base/include/ecal/measurement/base/types.h
[[deprecated("Please construct a Channel object explicitly.")]]
Channel(const std::string name_) : name(name_) {};

Channel(const std::string name_, id_t id_) : name(name_), id(id_) {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no type named 'string' in namespace 'std' [clang-diagnostic-error]

          Channel(const std::string name_, id_t id_) : name(name_), id(id_) {};
                             ^

contrib/measurement/base/include/ecal/measurement/base/types.h
//!< @endcond
};

inline Channel CreateChannel(const std::string& name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no type named 'string' in namespace 'std' [clang-diagnostic-error]

        inline Channel CreateChannel(const std::string& name)
                                                ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@FlorianReimold FlorianReimold Awaiting requested review from FlorianReimold

@rex-schilasky rex-schilasky Awaiting requested review from rex-schilasky

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@KerstinKeller