Fix stats print on ARM due to indeterminate state of "va_list" parameter after vasprintf (take 2) by stv0g · Pull Request #935 · VILLASframework/node

stv0g · 2025-06-27T08:23:55Z

This is another take on #933 / #678.

I think, I found a nice approach by pre-building the format string for a whole table row ince during the initialization. That allows us to use a single vsprintf call to format the whole row.

What do you think?

I also tweaked the table rendering a bit to fix some miscalculated column width.

Closes #678

n-eiling · 2025-06-27T08:51:34Z

If you are at it: Wouldn't it be nice to not use an allocating printf? We should be able to know the (max) column size and therefore should be able to use a fixed size buffer. Also, an option to buffer the output by pre-allocating a bunch of columns would be nice for real-time code. Or even pre-allocating all rows if we are running for a known time. With the current code, the stats hook is not usable in low time step real-time scenarios.

stv0g · 2025-06-27T12:08:59Z

@n-eiling Thanks for the rewiew. I rewrote it again to use fmtlib argument lists. This lets us ditch va_list completely. And we are using C++ templates for proper variadic functions.

Furthtermore, we should save a bunch of allocations as all of that is handled by fmtlib/spdlog.

IgnoreWarnings · 2025-06-27T13:42:51Z

Did you / would you like me to test this on HW?
Pipeline fails

IgnoreWarnings · 2025-06-27T13:43:56Z

@n-eiling Thanks for the rewiew. I rewrote it again to use fmtlib argument lists. This lets us ditch va_list completely. And we are using C++ templates for proper variadic functions.

Furthtermore, we should save a bunch of allocations as all of that is handled by fmtlib/spdlog.

I think the same about the allocations (as I answered in #933)

n-eiling · 2025-06-27T15:23:31Z

This still allocates in the real-time critical path, or not? I think fmt is better for sure. If we ever need to output data in real-time we probably should create a separate hook similar to what I implemented in DPsim (https://github.com/sogno-platform/dpsim/blob/master/dpsim/src/RealTimeDataLogger.cpp)

@IgnoreWarnings yes please test as much as possible :)

IgnoreWarnings · 2025-06-30T11:50:42Z

Ping me when you reache a final state, the I will test the branch on arm hardware.

stv0g · 2025-07-03T18:58:19Z

Ping me when you reache a final state, the I will test the branch on arm hardware.

I fixed the broken pre-commit check. The CI should be green in a few minutes.

From my side, this is ready for test. I have already validated the fix on a ARM64 system (Macbook M4 Pro with a Linux VM).

n-eiling · 2025-07-04T10:09:17Z

Just a note: I think testing adding a raspberry pi VM to the CI could be a good idea.

stv0g · 2025-07-04T11:03:59Z

Just a note: I think testing adding a raspberry pi VM to the CI could be a good idea.

We have a GitLab CI runner on a Raspberry Pi 5 in the ACS lab:
https://git.rwth-aachen.de/acs/public/villas/node/-/jobs/6431303

Its currently only building the code / Docker image. We dont do any tests on it so far.

IgnoreWarnings · 2025-07-07T13:29:01Z

Ping me when you reache a final state, the I will test the branch on arm hardware.

I fixed the broken pre-commit check. The CI should be green in a few minutes.

From my side, this is ready for test. I have already validated the fix on a ARM64 system (Macbook M4 Pro with a Linux VM).

The pipeline is still not happy :(.

stv0g · 2025-07-09T10:47:26Z

The pipeline is still not happy :(.

Okay, I fixed the code formatting again. Lets see what it brings.

stv0g · 2025-07-09T12:11:44Z

@IgnoreWarnings, the pipeline passed now :) Did you already had a chance to test it on real hardware?

n-eiling

Please wait for @IgnoreWarnings tests :)

IgnoreWarnings · 2025-07-09T13:46:07Z

@IgnoreWarnings, the pipeline passed now :) Did you already had a chance to test it on real hardware?

I checked out your branch and it doesnt build due to some rtc error. It seems like it is not related to your changes but some other update on the master. My Pi just crashed, I will send the compiler output tomorrow.

IgnoreWarnings · 2025-07-10T17:39:12Z

@stv0g
2 months ago you pushed S13d62e29c on master. From this commit I get a compiler error:

[build] [ 54%] Building CXX object lib/nodes/CMakeFiles/nodes.dir/webrtc.cpp.o
[build] .../node/lib/nodes/webrtc.cpp: In member function ‘virtual const std::string& villas::node::WebRTCNode::getDetails()’:
[build] .../node/lib/nodes/webrtc.cpp:191:42: error: ‘struct rtc::Reliability’ has no member named ‘maxRetransmits’
[build]   191 |                         *dci.reliability.maxRetransmits,
[build]       |                                          ^~~~~~~~~~~~~~
[build] gmake[2]: *** [lib/nodes/CMakeFiles/nodes.dir/build.make:440: lib/nodes/CMakeFiles/nodes.dir/webrtc.cpp.o] Error 1
[build] gmake[1]: *** [CMakeFiles/Makefile2:492: lib/nodes/CMakeFiles/nodes.dir/all] Error 2
[build] gmake: *** [Makefile:156: all] Error 2

n-eiling · 2025-07-11T07:09:10Z

I don't think this is related to this PR. The webrtc node type is notorious for not compiling on various platforms. I assume this is a version incompatibility? Maybe we should download and compile a specific version in cmake similar to libwebsockets...

IgnoreWarnings · 2025-07-11T15:14:42Z

I don't think this is related to this PR. The webrtc node type is notorious for not compiling on various platforms. I assume this is a version incompatibility? Maybe we should download and compile a specific version in cmake similar to libwebsockets...

It isn't caused by this PR but prevents me from testing. Also the master should work (again) on pi.

stv0g · 2025-07-14T04:53:16Z

@IgnoreWarnings Which version of libdatachannel are you using?

Our deps.sh scripts is installing v0.22.6:

node/packaging/deps.sh

Line 459 in 77c4b44

    
           git clone ${GIT_OPTS} --recursive --branch v0.22.6 https://github.com/paullouisageneau/libdatachannel.git

This version includes the missing struct field which raises the error you posted above.

However, I noticed that we are not enforcing a version in the CMakeLists.txt:

node/CMakeLists.txt

Line 76 in 77c4b44

find_package(LibDataChannel)

Similarily, also the the deps.sh is not performing a version check.. But just checks for availability.. Thats something we should fix..

stv0g · 2025-07-14T04:53:38Z

@IgnoreWarnings Could you test it with cmake .. -DWITH_WEBRTC=OFF ?

IgnoreWarnings · 2025-07-14T13:27:11Z

@IgnoreWarnings Which version of libdatachannel are you using?

Our deps.sh scripts is installing v0.22.6:

node/packaging/deps.sh

Line 459 in 77c4b44

git clone ${GIT_OPTS} --recursive --branch v0.22.6 https://github.com/paullouisageneau/libdatachannel.git

This version includes the missing struct field which raises the error you posted above.

However, I noticed that we are not enforcing a version in the CMakeLists.txt:

node/CMakeLists.txt

Line 76 in 77c4b44

find_package(LibDataChannel)

Similarily, also the the deps.sh is not performing a version check.. But just checks for availability.. Thats something we should fix..

I fixed the compiler error by upgrading from 0.18.4 to 0.22.6 and pinning the version in cmake find package.
Adding the version check/update to deps.sh / cmake would be nice to prevent such errors.

IgnoreWarnings · 2025-07-14T13:28:46Z

@stv0g
commit bd505a3 (HEAD -> fix-table-print-arm, origin/fix-table-print-arm)

13:26:52 info stats             Node          │          Recv │          Sent │          Drop │          Skip │     OWD last │     OWD mean │    Rate last │    Rate mean │     Age mean │      Age Max │ Signals
13:26:52 info stats                           │          pkts │          pkts │          pkts │          pkts │         secs │         secs │      pkt/sec │      pkt/sec │         secs │          sec │ signals
13:26:52 info stats            ───────────────┼───────────────┼───────────────┼───────────────┼───────────────┼──────────────┼──────────────┼──────────────┼──────────────┼──────────────┼──────────────┼─────────
[*** LOG ERROR #0001 ***] [2025-07-14 15:26:53] [stats] {unmatched '}' in format string}
[*** LOG ERROR #0003 ***] [2025-07-14 15:26:55] [stats] {unmatched '}' in format string}
[*** LOG ERROR #0005 ***] [2025-07-14 15:26:57] [stats] {unmatched '}' in format string}
[*** LOG ERROR #0006 ***] [2025-07-14 15:26:58] [stats] {unmatched '}' in format string}
^C13:26:59 info node             Received Interrupt signal. Terminating...

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

stv0g · 2025-07-15T09:16:51Z

Oops my bad

IgnoreWarnings · 2025-07-15T09:57:58Z

@stv0g pipeline failed integration timing, can you restart it?

n-eiling · 2025-07-15T14:01:22Z

@IgnoreWarnings I restarted it. It's ok now.

IgnoreWarnings · 2025-07-15T16:15:40Z

@stv0g commit bd505a3 (HEAD -> fix-table-print-arm, origin/fix-table-print-arm)

13:26:52 info stats             Node          │          Recv │          Sent │          Drop │          Skip │     OWD last │     OWD mean │    Rate last │    Rate mean │     Age mean │      Age Max │ Signals
13:26:52 info stats                           │          pkts │          pkts │          pkts │          pkts │         secs │         secs │      pkt/sec │      pkt/sec │         secs │          sec │ signals
13:26:52 info stats            ───────────────┼───────────────┼───────────────┼───────────────┼───────────────┼──────────────┼──────────────┼──────────────┼──────────────┼──────────────┼──────────────┼─────────
[*** LOG ERROR #0001 ***] [2025-07-14 15:26:53] [stats] {unmatched '}' in format string}
[*** LOG ERROR #0003 ***] [2025-07-14 15:26:55] [stats] {unmatched '}' in format string}
[*** LOG ERROR #0005 ***] [2025-07-14 15:26:57] [stats] {unmatched '}' in format string}
[*** LOG ERROR #0006 ***] [2025-07-14 15:26:58] [stats] {unmatched '}' in format string}
^C13:26:59 info node             Received Interrupt signal. Terminating...

@stv0g I still get the same error.
commit dbd4f5a (HEAD -> fix-table-print-arm, origin/fix-table-print-arm)

IgnoreWarnings · 2025-07-15T16:19:07Z

I heared you want to roll out soon and just wanted to remind you (since this PR was already opened 3 weeks ago) that I have a confirmed working hotfix in #933.

pjungkamp

I don't know whether the fmt::runtime_format_string<> type could be used as the type of the Table::rowFormat class member variable. It might be worth a try if this saves time in the Table::row.

Signed-off-by: Niklas Eiling <niklas@eil.ing>

…dards Co-authored-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Niklas Eiling <niklas@eil.ing>

stv0g · 2025-07-16T07:10:00Z

Thanks for you support 👍🏻

stv0g · 2025-07-16T07:10:31Z

Do you mind approving, so we can merge once the CI passes?

n-eiling · 2025-07-16T07:11:22Z

Let's wait for @IgnoreWarnings to check if everything works.

IgnoreWarnings · 2025-07-16T07:55:10Z

Let's wait for @IgnoreWarnings to check if everything works.

Will test in couple of hours.

IgnoreWarnings · 2025-07-16T10:31:21Z

The numbers look right now on arm. The table header isnt aligned perfectly, but i think it wasnt aligned before.

10:28:26 info stats             Node          │          Recv │          Sent │          Drop │         Skip │     OWD last │     OWD mean │    Rate last │    Rate mean │     Age mean │      Age Max │ Signals
10:28:26 info stats                           │          pkts │          pkts │          pkts │         pkts │         secs │         secs │      pkt/sec │      pkt/sec │         secs │          sec │ signals
10:28:26 info stats            ───────────────┼───────────────┼───────────────┼───────────────┼──────────────┼──────────────┼──────────────┼──────────────┼──────────────┼──────────────┼──────────────┼─────────
10:28:27 info stats             signal_gen_0 │             0│             0│             0│            0│     0.000000│          nan│          inf│          nan│          nan│     0.000000│       1
10:28:28 info stats             signal_gen_0 │             1│             0│             0│            0│     0.999832│     0.999832│     1.000008│     1.000008│          nan│     0.000000│       1
10:28:29 info stats             signal_gen_0 │             2│             0│             0│            0│     0.999904│     0.999868│     1.000000│     1.000004│          nan│     0.000000│       1
10:28:30 info stats             signal_gen_0 │             3│             0│             0│            0│     0.999904│     0.999880│     1.000000│     1.000003│          nan│     0.000000│       1

stv0g requested a review from IgnoreWarnings June 27, 2025 08:23

stv0g self-assigned this Jun 27, 2025

stv0g requested a review from n-eiling as a code owner June 27, 2025 08:23

stv0g mentioned this pull request Jun 27, 2025

Fix #678: Fix stats print on ARM due to indeterminate state of "va_list" parameter after vasprintf #933

Closed

stv0g force-pushed the fix-table-print-arm branch from e9c0464 to 604f80e Compare July 3, 2025 18:57

stv0g force-pushed the fix-table-print-arm branch 2 times, most recently from bfdafa0 to bd505a3 Compare July 9, 2025 10:46

stv0g changed the title ~~Fix #678: Fix stats print on ARM due to indeterminate state of "va_list" parameter after vasprintf (take 2)~~ Fix stats print on ARM due to indeterminate state of "va_list" parameter after vasprintf (take 2) Jul 9, 2025

n-eiling previously approved these changes Jul 9, 2025

View reviewed changes

n-eiling reviewed Jul 14, 2025

View reviewed changes

Comment thread common/lib/table.cpp Outdated

steffen-vogel-opalrt added 5 commits July 15, 2025 11:14

fix(table): Improve table printing for ARM architecture

1fc2ca1

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(stats,table): Improve table rendering

cd87bf4

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(config): Make example config for stats hook more useful

a43399f

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(table): Avoid allocations

e24fb7d

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(lint): Format code with clang-format

dbd4f5a

Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

stv0g force-pushed the fix-table-print-arm branch from bd505a3 to dbd4f5a Compare July 15, 2025 09:20

pjungkamp requested changes Jul 15, 2025

View reviewed changes

Comment thread common/include/villas/table.hpp Outdated

Comment thread common/include/villas/log.hpp Outdated

Comment thread common/lib/table.cpp Outdated

Comment thread common/lib/table.cpp

n-eiling and others added 2 commits July 16, 2025 08:57

fix: remove unecessary } in format string in table.cpp

3c999f7

Signed-off-by: Niklas Eiling <niklas@eil.ing>

fix: improve compatiblity of villas::utils::Table with newer C++ stan…

8207491

…dards Co-authored-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Niklas Eiling <niklas@eil.ing>

n-eiling dismissed their stale review via 8207491 July 16, 2025 07:08

stv0g requested review from n-eiling and pjungkamp July 16, 2025 07:10

pjungkamp approved these changes Jul 16, 2025

View reviewed changes

IgnoreWarnings approved these changes Jul 16, 2025

View reviewed changes

stv0g enabled auto-merge (rebase) July 16, 2025 11:05

stv0g disabled auto-merge July 16, 2025 11:05

n-eiling approved these changes Jul 16, 2025

View reviewed changes

n-eiling merged commit 8bc146a into master Jul 16, 2025
3 checks passed

n-eiling deleted the fix-table-print-arm branch July 16, 2025 11:40

Conversation

stv0g commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n-eiling commented Jun 27, 2025

Uh oh!

stv0g commented Jun 27, 2025

Uh oh!

IgnoreWarnings commented Jun 27, 2025

Uh oh!

IgnoreWarnings commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n-eiling commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgnoreWarnings commented Jun 30, 2025

Uh oh!

stv0g commented Jul 3, 2025

Uh oh!

n-eiling commented Jul 4, 2025

Uh oh!

stv0g commented Jul 4, 2025

Uh oh!

IgnoreWarnings commented Jul 7, 2025

Uh oh!

stv0g commented Jul 9, 2025

Uh oh!

stv0g commented Jul 9, 2025

Uh oh!

n-eiling left a comment

Choose a reason for hiding this comment

Uh oh!

IgnoreWarnings commented Jul 9, 2025

Uh oh!

IgnoreWarnings commented Jul 10, 2025

Uh oh!

n-eiling commented Jul 11, 2025

Uh oh!

IgnoreWarnings commented Jul 11, 2025

Uh oh!

stv0g commented Jul 14, 2025

Uh oh!

stv0g commented Jul 14, 2025

Uh oh!

IgnoreWarnings commented Jul 14, 2025

Uh oh!

IgnoreWarnings commented Jul 14, 2025

Uh oh!

Uh oh!

stv0g commented Jul 15, 2025

Uh oh!

IgnoreWarnings commented Jul 15, 2025

Uh oh!

n-eiling commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgnoreWarnings commented Jul 15, 2025

Uh oh!

IgnoreWarnings commented Jul 15, 2025

Uh oh!

pjungkamp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stv0g commented Jul 16, 2025

Uh oh!

stv0g commented Jul 16, 2025

Uh oh!

n-eiling commented Jul 16, 2025

Uh oh!

IgnoreWarnings commented Jul 16, 2025

Uh oh!

IgnoreWarnings commented Jul 16, 2025

Uh oh!

Uh oh!

Reviewers

stv0g commented Jun 27, 2025 •

edited

Loading

IgnoreWarnings commented Jun 27, 2025 •

edited

Loading

n-eiling commented Jun 27, 2025 •

edited

Loading

n-eiling commented Jul 15, 2025 •

edited

Loading