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.

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

Get mma linux more generic #1284

Merged
merged 21 commits into from
Dec 22, 2023
Merged

Conversation

jheeks
Copy link
Contributor

@jheeks jheeks commented Dec 18, 2023

Description

  • remove dependency to sysstat and iostat
  • get most data from /proc
  • consider all mounted disks
  • results in better support of embedded devices

Related issues

Cherry-pick to

  • none

* Does not depend on helper tools like sar or top
* Uses the second value which is the idle time of all cores in seconds
* Does not depend on helper function ifstat
* Does not depend on helper tool iostat
* Does not depend on helper tool ps
to be more generic
because /dev/root is a kind of virtual device
* consider all mounted disks
* sum up numbers for partitions of same disk
* special handling for mtdblock devices
* avoid division by zero
* garbage collection for process utime data
from /etc/os-release
* remove unused code
* add more comments
* improve code readability
* runtime check if vcgencmd is available
* not having this behind compile flag
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 22. Check the log or trigger a new build to see more.

contrib/mma/include/linux/mma_linux.h Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@hannemn hannemn left a comment

Choose a reason for hiding this comment

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

Thanks for reworking the MMA and for getting rid of sysstat and ifstat dependencies. I just did a quick test on my x86 machine and everything still works as expected. Once the coding style findings are reworked, we can bring your adaptions into the master.

proc_prev_count_ = item.count;

// garbage collection
for (auto it = proc_prev_map.begin(), next = it; it != proc_prev_map.end(); it = next)
Copy link
Contributor

Choose a reason for hiding this comment

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

Iterating on a map and at the same time removing certain elements can be dangerous. I am not 100% sure if your code can lead to unintended side-effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation follows a pattern given on Stackoverflow here: https://stackoverflow.com/a/39719450
The idea is to get the next iterator already before erasing the current entry.
The erased entries belong to old processes which are finished, means not listed in /proc/[pid] any more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see and acually thought there is a std::erase_if() functionality from the library available that can be used here. But it turned out this helper method comes along with C++20 which is too "new" for our code base.

Let member variables end with `_`
@jheeks jheeks requested a review from hannemn December 20, 2023 13:10
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/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
contrib/mma/src/linux/mma_linux.cpp Outdated Show resolved Hide resolved
@hannemn hannemn merged commit cd67b47 into eclipse-ecal:master Dec 22, 2023
8 checks passed
@jheeks jheeks deleted the mmaLinuxGeneric branch December 28, 2023 16:59
@DownerCase
Copy link
Contributor

  • consider all mounted disks

Thank you this specific change! I was recording to a "data drive" and was sad when mma wasn't reporting the usage of my recording drive..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants