bug fixing and improvements

[R-Abbasi]

• improved the code to save measurements for all connected devices
• rather than only doing one async task at a time, launched a single thread that does all the work more efficiently
• used std::atomic to make variables thread-safe
• live devices data extraction is done only when there is/are some file(s) loaded
• code cleanup

It's expected that you have, e.g., three saved files (one progress bar for each) containing related measurement data if you connect 3 devices and measure data using them

[AKMaily]

I connected four devices, started the measurement, stopped the measurement, selected all 4 devices in the save menu and clicked on the save button. The software closed. After a few seconds a saves folder was created in the file browser, in this saves folder was one file which saved the data of the fourth device. Tried it a second time, same behavior. The third time I tried the application closed but there were two files with the same amount of data from the fourth and third devices. These are the connected devices: These are the files that were saved:

[R-Abbasi]

Since the behavior varies, it must be related to the threads. Do the following steps, please: 1- create an empty folder on, say, Desktop, then: connect two devices -> make some measurements for both -> Save -> give the empty folder's path to both devices -> check both devices -> take a screenshot

2- Save the files -> open the empty folder -> take another screenshot Then send both screenshot, please.

[AKMaily]

This is the Software when two devices are connected and an empty folder is selected: This is the output in the filebrowser: This is the format of the saved file:

[R-Abbasi]

Will send you another executable. Clean the initial empty folder and do the test again, please.

[AKMaily]

These are the settings in the software :

When i click on save the software closes. I cant make a screenshot because the console also closes but it printed: Finished saving.

One file is saved in the directory:

This is the output in the file :

[AKMaily]

Tested the application again with the same userflow. Now the application closes when pressing the save button and no file is saved.

[R-Abbasi]

Test it with only one device, please.

[AKMaily]

It has the same behavior. The application closes and no file is saved.

[R-Abbasi]

It works fine on my system.

[R-Abbasi]

Here's a screenshot of the Debug mode that works fine:

Will send you the file and test it first with one device and then two, ..., please.

[AKMaily]

Tested the application serveral times. Connecting one device and trying to save the data via the save menu leads to different behaviors:

1. in one case the software behaved as it should and saved the file in the correct folder. I could not replicate this again.
2. in one case the software closes and saves no file
3. in the third case pressing the save button freezes the application

Connecting two devices leads to the behavior of point 2 and 3.

[DanielNowak98]

I just tested the file-save bugfixing with 4 Scopes. The ammount of data is now similar.

The CSV-File, that was created is not a "real" - csv.

[R-Abbasi]

If we connect 4 devices and start measuring, there should be the same amount of data in four files for them, presumably. I don't see a problem, do you, please?

The CSV-File, that was created is not a "real" - csv.

What do you mean by that, please?

[DanielNowak98]

If we connect 4 devices and start measuring, there should be the same amount of data in four files for them, presumably. I don't see a problem, do you, please?

The CSV-File, that was created is not a "real" - csv.

What do you mean by that, please?

If we connect 4 devices and start measuring, there should be the same amount of data in four files for them, presumably. I don't see a problem, do you, please?

In this version, everything works as intended. Previously, the amount of data in the CSV files was either inconsistent or empty.

The CSV-File, that was created is not a "real" - csv.

What do you mean by that, please?

A typical CSV file consists of rows, with each row representing a data record, and columns separated by commas. Each row should have the same number of columns. Ideally, the CSV file should look like this if you want to give metadata like Device ID and Sampling rate.

# Metadata
# Date: 2024-06-26
# Device: E4620C205B122E34
# File: device1-2024-06-26T09-18.csv
# Total Rows: 100000

# Data
Voltage,
50,
49,
50,

[R-Abbasi]

Having all metadata on the first line and _yvalues (voltages) start from the second line with each on a new line were already agreed upon. If you note, that part hasn't even been touched.

[DanielNowak98]

Having all metadata on the first line and _yvalues (voltages) start from the second line with each on a new line were already agreed upon. If you note, that part hasn't even been touched.

I didn't know that this had already been agreed upon. I'm sorry about that. My suggestion was intended as an improvement to create a structure of the data that would be easier for others to understand.

[R-Abbasi]

I suggest that you create an issue for that and explain what is not quite right now and how your idea is going to solve it.

[AKMaily]

Tested the newest version of the application. I get the following cases:

Connected Devices : 4 without a voltage source

Buttons: Search for Devices --> Start --> Stop --> Save

Checking all four devices for saving

Output in the saved folder are four files with the same amount of data :

Tested this a few times with the same userflow and it seems to work fine and the data files seem to be saved correctly:

Found Bug: Sometimes when clicking on "Search Devices" the application closes. Please check is this is a bug that could be caused by the code changes

Second case :

Connected devices: 4

2 devices with voltage source

Buttons: Same behavior

Output: The two devices with a voltage source have a different amount of data than those without one :

This is shown in the software :

Disconnecting the voltage source and measuring again with 4 devices with no voltage source has the following output :

Important are here the files that were saved at 9:46

This is shown in the software :

Starting the application again resets the bug and the files are saved again with the same amount of data as long as there is no voltage source connected :

Case 3 :

Connected devices : 2 with voltage source

Buttons: Same behavior

Output :

I zoomed in the graph to see if that the scopes are synched and actually measure the same amount of data :

At the start point:

At the endpoint :

But not the same amount of data is saved in the files :

After that i connected two different devices with no voltage source :

Which corresponds in two files with the same amount of data:

Reconnecting the two devices that before had a voltage source again without a voltage source, taking a measurement and saving it ends in two files with the same amount of data.

Bug: The data is overwritten sometimes if i take a new measurement with the same devices and save it again. I am not sure if the file is complety overwritten or it the data is added at the bottom of the file because the files get larger. Please check if this could be caused by the code changes.

[R-Abbasi]

It seems you're getting each and any case, which may not even be used when working with the program, involved and altogether.

Found Bug: Sometimes when clicking on "Search Devices" the application closes. Please check is this is a bug that could be caused by the code changes

That part hasn't been changed iirc. And that issue has already happened at times. I guess it belongs to the cables/scope.

Connected devices: 4 2 devices with voltage source Buttons: Same behavior Output: The two devices with a voltage source have a different amount of data than those without one :

If you by "with voltage source" mean the time when the signal generator is turned on, then the waves are different when the signal generator is on compared to the time it's off while you're making a measurement. Hence I guess it's expected to have different data.

Disconnecting the voltage source and measuring again with 4 devices with no voltage source has the following output Important are here the files that were saved at 9:46

Didn't get that part, especially this you measure without a source.

But not the same amount of data is saved in the files

The sizes look quite close to each other and am not sure what it has anything to do with what I implemented.

Bug: The data is overwritten sometimes if i take a new measurement with the same devices and save it again. I am not sure if the file is complety overwritten or it the data is added at the bottom of the file because the files get larger. Please check if this could be caused by the code changes.

This is not a bug either. Take a look at how files are created, please.

[AKMaily]

It seems you're getting each and any case, which may not even be used when working with the program, involved and altogether.

Testing for every possible case, especially when working with Threads is common procedure. It is needed to see if the saving process works right and the issue is fixed.

The issue is resolved, when in every case where different OmnAIScopes are connected the files are saved with the same amount of data into a different file. Every time the user saves a file there should be a new file created where this data is saved, no files should be overwritten.

This means the definition of done is:

1. A measurement taken by one connected scope can be saved in a new .csv file at the given path. Every data point is saved correctly. The software does not close or crash.

2. A measurement taken by multiple scopes can be saved in separate new .csv files at the given path. A new file is created for each scope. Every data point is saved correctly. The software does not close or crash. This implies that for 4 connected scopes (with or without a voltage source), 4 files are saved, one for each scope, and all files contain the same amount of data.

3. The code written for this is clean and readable. Variable names are descriptive of their usage, and the files are formatted with clang-format.

Found Bug: Sometimes when clicking on "Search Devices" the application closes. Please check is this is a bug that could be caused by the code changes

That part hasn't been changed iirc. And that issue has already happened at times. I guess it belongs to the cables/scope.

Okay I will adress this in a second issue.

Connected devices: 4 2 devices with voltage source Buttons: Same behavior Output: The two devices with a voltage source have a different amount of data than those without one :

If you by "with voltage source" mean the time when the signal generator is turned on, then the waves are different when the signal generator is on compared to the time it's off while you're making a measurement. Hence I guess it's expected to have different data.

With voltage source i mean that the Scopes are connected to a voltage source, in this case a signal generator. The y-values that are measured are different, because they depend on the connected voltage. However the amount of data that is measured is not different as the Scopes are synchronized and all measure the same amount of x-values in the same range. Therefore the files should have the same size.

Disconnecting the voltage source and measuring again with 4 devices with no voltage source has the following output Important are here the files that were saved at 9:46

Didn't get that part, especially this you measure without a source.

As all the files have different amount of data, in the picture it is not clear which of the files are the ones from the measurement without a source. I wanted to point out that the first measurement at 9:44 o'clock was the first one with a voltage source and the second measurement where all 4 devices are not connected to a voltage source are the files that were saved at 9:46 o'clock. All files have different amount of data, even after disconnecting the voltage source, which indicates that the bug is not only dependend on the y-values that are measured.

But not the same amount of data is saved in the files

The sizes look quite close to each other and am not sure what it has anything to do with what I implemented.

Important is that the files should have the exact same amount of data. Not quite close.

This has actually everything to do with what you implemented as it the bug we want to fix. Two devices where connected with a voltage source, a measurement was taken, the measurement is saved but the files do not contain the same amount of data.

If anything is unclear please feel free to ask in the comments.

Bug: The data is overwritten sometimes if i take a new measurement with the same devices and save it again. I am not sure if the file is complety overwritten or it the data is added at the bottom of the file because the files get larger. Please check if this could be caused by the code changes.

This is not a bug either. Take a look at how files are created, please.

The definition of a bug is: "A bug is an error, flaw, or fault in a software program that causes it to produce incorrect or unexpected results or to behave in unintended ways."

As the programm is expected to save every measurement in a different file for each scope, overwritting an existing file with a different measurement is an unexpected and unitended behavior and results in an unexpected result. Therefor it is a bug.

[R-Abbasi]

Testing for every possible case, especially when working with Threads is common procedure.

Testing different use cases in connection to the task is expected but it mustn't necessarily pertain to threads. We've also the main thread already there.

Every time the user saves a file there should be a new file created where this data is saved, no files should be overwritten.

As all the files have different amount of data, in the picture it is not clear which of the files are the ones from the measurement without a source. I wanted to point out that the first measurement at 9:44 o'clock was the first one with a voltage source and the second measurement where all 4 devices are not connected to a voltage source are the files that were saved at 9:46 o'clock. All files have different amount of data, even after disconnecting the voltage source, which indicates that the bug is not only dependend on the y-values that are measured.

Again, if you look at how files are made, you'll get that, I assume.

However the amount of data that is measured is not different as the Scopes are synchronized and all measure the same amount of x-values in the same range. Therefore the files should have the same size.

The tiny difference in size may connect to some details regarding those different waves. You can open the files when specifically doing a test for that, I suppose.

This has actually everything to do with what you implemented as it the bug we want to fix. Two devices where connected with a voltage source, a measurement was taken, the measurement is saved but the files do not contain the same amount of data.

You and already Daniel tested the program with 4 devices and both pointed out that it works properly. Which part of the code is suspicious, please?
It's not very constructive to just say there's a bug here and there without understanding the code completely and having correct testing, to me.

The definition of a bug is: "A bug is an error, flaw, or fault in a software program that causes it to produce incorrect or unexpected results or to behave in unintended ways."

The point is not what a bug is; the point is that you seemingly didn't observe this obvious thing that files are created this way based on the current implementation and that part hasn't even been touched in this pr.

[AKMaily]

Every time the user saves a file there should be a new file created where this data is saved, no files should be overwritten.

As all the files have different amount of data, in the picture it is not clear which of the files are the ones from the measurement without a source. I wanted to point out that the first measurement at 9:44 o'clock was the first one with a voltage source and the second measurement where all 4 devices are not connected to a voltage source are the files that were saved at 9:46 o'clock. All files have different amount of data, even after disconnecting the voltage source, which indicates that the bug is not only dependend on the y-values that are measured.

Again, if you look at how files are made, you'll get that, I assume.

It is not about the way those files are saved. It is about the way they should be saved. I looked into the files and when a voltage is connected, they seem to have missing datapoints as you can see here : This could be the reason for the different amount of data that is saved.

However the amount of data that is measured is not different as the Scopes are synchronized and all measure the same amount of x-values in the same range. Therefore the files should have the same size.

The tiny difference in size may connect to some details regarding those different waves. You can open the files when specifically doing a test for that, I suppose.

As explained above.

This has actually everything to do with what you implemented as it the bug we want to fix. Two devices where connected with a voltage source, a measurement was taken, the measurement is saved but the files do not contain the same amount of data.

You and already Daniel tested the program with 4 devices and both pointed out that it works properly. Which part of the code is suspicious, please? It's not very constructive to just say there's a bug here and there without understanding the code completely and having correct testing, to me.

First of all, when testing the application, it is not necessary for me to know how the code works; that is not my responsibility. My job is to test the use cases of the application and verify that it functions correctly. Understanding how the code is implemented, identifying where the bug is, and addressing issues in your pull requests is your responsibility. While I am happy to assist with this, it is not my primary task.

I understand that this issue is frustrating, especially since you cannot test the cases yourself at the moment.

Secondly, both Daniel and I tested the program. As you can see, Daniel tested only one case where no negative numbers occur. I tested several cases and, as described in my review, not all of them resulted in the expected behavior. Moreover, they caused various different behaviors depending on the case. Therefore, the bug is not fixed.

If there is anything unclear, please feel free to ask in the comments.

[AKMaily]

Furthermore it seems to be a problem with the negative values that are measured:

I tested the application with a positive voltage and the files are saved correctly:

Measurement at 16:56 o'clock Although displaying the data in the software again, they seem to get an offset on the x-axis.

[R-Abbasi]

Files overriding case can occur with the current implementation. As mentioned, an issue is to be created if that should change.

Don't actually see any clear issue with the implementation from https://github.com/skunkforce/OmniView/pull/167/commits/d4a1262001df521df0d413c7d955736c943ec4b5. Even https://github.com/skunkforce/OmniView/pull/167/commits/c4bbbe74fdb26ce1114ba8797d916f7546f32864 was not to fix an issue but mainly for memory usage and performance purposes.

Took four tests while the signal generator was on for two of them and off for the other two, then sorted their values. There was no negative or missing _yvalue in any of the four files. It's not completely clear for me how you test the program to reproduce results you talked about.

My signal generator settings thus far:

[AKMaily]

Please provide a screenshot of the application where the waveforms you measured with your devices are shown as well as an example screenshot of the data that is saved in the files.

[R-Abbasi]

Here's the screenshot when the signal generator is on. When sorted, as shown, there's no negative or missing elements (sorting starts from the first line that's why you see one missing row for each file):

The second screenshot when the signal generator is off:

[AKMaily]

Here's the screenshot when the signal generator is on. When sorted, as shown, there's no negative or missing elements (sorting starts from the first line that's why you see one missing row for each file):

If you are measuring a waveform that has no negative y-values, as you can see in the plotwindow of the screenshot you provided, there will be no negativ y-values in the file. You have to take a measurement with a signal that has negative number of counts to reproduce this behavior.

[R-Abbasi]

Perhaps your signal generator uses a different settings that produces negative values, not sure though. However, resize_and_overwrite works fine for negative values too, apparently.

Yet it's a little astonishing for me why negative _yvalues are required and how to make the signal generator produce them instead of positive values.

As well as, it sounds like a separate/new feature as it's the first time I hear the code should handle negative _yvalues too.

[AKMaily]

As this issue turns out to be a lot of seperate bugs that cant be solved within this one PR, i will create seperate issues for the adressed problems. This should make the testing and the implementation easier and less complex.

[R-Abbasi]

1- As our approach is "one new feature/issue -> one pr" and it has been mentioned a couple of times, it's not very reasonable to expect a pr to do multiple things, I suppose.

2- File overriding, when you save a second measurement quickly after the first one, has been eariler implemented deliberately. It doesn't seem the first implementer didn't know what the code is going to do. That's why I won't call it a bug. Changing it will be more like a new feature, to me.

3- Right now the program supports negative values - already provided you with a link. That will also be a new feature if wider ranges need to be supported, I believe.

[R-Abbasi]

Sorry, still don't understand the issue.

The bug belonged to objects lifetime which was fixed from the third commit on. The lambda introducer has =, &liveDvcs = std::as_const(liveDvcs) in which = captures by const copy and &liveDvcs = std::as_const(liveDvcs) captures by const reference for which there's already a comment. Then the _forloop begins which has some the before-mentioned commands. .get() is also called when if (saved_files_cnt == checked_devices_cnt) evaluates to true. There's in advance a comment for that as well. If there's anything you'd like to know more, let me know, please, and I'll try explaining more.

Lack of a required comment or adding a redundant one makes the code not good, I think.

[AKMaily]

Like this :

future = std::async( // const reference to the container
std::launch::async, [=, &liveDvcs = std::as_const(liveDvcs)] {  // create a file for each selected device 
          // Iterate through each device and its values in liveDvcs
          for (size_t i{}; const auto &[device, values] : liveDvcs) {
            // Check if the device at index i is checked
            if (dvcCheckedArr[i].b) {
              fs::path path;
              // Create a filename based on the device index
              auto filename = mkFileName(fmt::format("device{}", i + 1));

              // If a selected path is available for the device, use it
              if (hasSelectedPathArr[i].b) {
                path = mkdir(true, selectedPathArr[i], "", filename);
                hasSelectedPathArr[i].b = false;
              } 
              // If there is input in the text fields, use it as the path
              else if (!inptTxtFields[i].empty()) {
                path = mkdir(false, "", inptTxtFields[i], filename);
                inptTxtFields[i].clear();
              } 
              // Otherwise, use the default path
              else
                path = mkdir(false, "", "", filename);

              // Get the size of the values
              valuesSize = values.size();

              // Save the device data to the path, important: for each device a seperate file is created
              save(device, values, path, allData, y_indx, filename, saved_files_cnt);
            }
            i++;
          }
        });

[R-Abbasi]

std::launch::async, [=, &liveDvcs = std::as_const(liveDvcs)] { // create a file for each selected device

No file is created here.

// Iterate through each device and its values in liveDvcs

A loop means iteration by itself primarily

// Check if the device at index i is checked

dvcCheckedArr[i] expresses that vividly. b can be seen easily too.

// Create a filename based on the device index

mkFileName points that out: make file name

// If a selected path is available for the device, use it

hasSelectedPathArr says that too.

// If there is input in the text fields, use it as the path

!inptTxtFields[i].empty() doesn't seem insufficient. As a rule of thumb, we use expressive identifiers to get rid of commenting each and every expression/line like above.

// Otherwise, use the default path

mkdir has meaningful names for its parameters.

// Get the size of the values

This is what valuesSize and the assignment are obviously doing.

// Save the device data to the path, important: for each device a seperate file is created

The function name is already save. It's called on each iteration of the loop meaning it's called for each device measurement for which the device is selected.

[AKMaily]

https://opensource.com/article/17/10/source-code-comment-poll Please read the article as well as the comments. You will find that this discussion could go on and on. As this code is not only used by you and me but also other developers outside and inside the company, it is necessary to comment the code in a good way.

[R-Abbasi]

I think commenting almost each and every line of code, like what you added, isn't quite good. Here's also what Stroustrup says in A Tour of C++ 3rd Edition, Basic section, page 20

[35] Don’t say in comments what can be clearly stated in code; [CG: NL.1].

However, adding some comments like below can be fine, to me:

if (dvcCheckedArr[i].b) {  // save measurement only if device is checked 
  // ...
  // save the measurement 
  save(device, values, path, allData, y_indx, filename, saved_files_cnt);
  // ...

[AKMaily]

As long as it is clearly commented in the code that the data of each device is saved in a seperate file and where this happens, i am fine with it

[R-Abbasi]

if (dvcCheckedArr[i].b) {  // measurement saving preparation if device is checked 
  // ...
  // save measurement in a separate file  
  save(device, values, path, allData, y_indx, filename, saved_files_cnt);
  // ...

Where files are created is clear by the function call save(...)

[AKMaily]

You can discuss this in length, this will not change the fact that this PR will not be merged until the comments are set correct. Comment the part where the devices are saved, comment that each device is saved in its own file.

[R-Abbasi]

By my recent comment above, what the code is going to do must be completely clear. Am not sure why you're insisting on changing it. Pay attention to what Stroustrup says, please. He's the creator of C++.

[AKMaily]

if (dvcCheckedArr[i].b) { // measurement saving preparation if device is checked // ... // save measurement of each device in a separate file
save(device, values, path, allData, y_indx, filename, saved_files_cnt); // ...

[R-Abbasi]

The pr could be merged at https://github.com/skunkforce/OmniView/pull/167#issuecomment-2196586803 but was extended for no clear reason, to me. Anyway, added those few words too.