Benchmarking #526

g0nz4I0 · 2025-09-26T17:26:07Z

Added SEGGER_RTT files to project
Added SEGGER_RTT files to CMakeLists.txt
Fixes to DataWatchpointTrace module
IFDEFs guards for Benchmarking mode

Benchmarking Features

Different types of Events:

For measuring time executions of code blocks
Measuring time between different executions

This is achieved by a set of macros, which record the current CycleCounter at a given point.

Under the hood, the macros use a string which is used by the frontend to display pretty messages.
To communicate with the host PC it uses SEGGER_RTT protocol, so it needs to have a PC hooked at all times.
It introduces breaking changes by changing the software used to flash, moving away from STLINK to OPENOCD.

It also adds some polling checks to detect overflow and update the global counter.

…orking beforehand, take a look at the old reset_cnt disabling DWT_CTRL_CYCCNTENA by setting DWT->CTRL to 0)

… overflow check in STLIB::update()

…Benchmarking as well

jorgesg82 · 2025-12-14T11:20:03Z

It would be also really really nice to have a doc folder hooked in the root one, and a benchmarking.md explaining how this work/how to use it, and link this file from the README

jorgesg82 · 2025-12-14T11:33:01Z

Well, indeed, there is a wiki...
https://github.com/HyperloopUPV-H8/ST-LIB/wiki

g0nz4I0 · 2025-12-16T15:36:09Z

I missed the comment, sorry, maybe I can add it to this wiki?

victor-Lopez25 · 2025-12-26T00:03:03Z

Would it be possible to add in the DWT CPI counter to get info on additional cycles spent on IF stalls or multicycle instructions?
It would be better to get info on the amount of cache misses in a block of code too but it doesn't look like there's any way in STM32H723.
The CPI counter is only 8 bits but you can enable an overflow event.

g0nz4I0 · 2025-12-26T01:19:45Z

The DWT peripheral can do a bunch of stuff yes, but I havent been able to make it work with VSCode yet, the event is not a SW event, but rather an internal event that triggers a transfer via SWD, this works fine in STM32CubeIDE, but I havent been able to make it work in VScode or outside the IDE yet, it's a work in progress

I can say however that the cache thing is impossible, some things that are acctually possible are checking cycles spent in interrupts or sleeeping for example

Copilot

Pull request overview

This PR adds benchmarking infrastructure to the project by integrating SEGGER RTT (Real Time Transfer) for communication with a host PC and implementing performance measurement capabilities through the ARM Data Watchpoint and Trace (DWT) unit.

Key changes:

Integration of SEGGER RTT third-party library for real-time data transfer
New benchmarking toolkit with DWT-based cycle counting and RTT-based logging
Refactored DataWatchpointTrace class with overflow detection support
Build system updates to include RTT sources and headers

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
Src/HALAL/HALAL.cpp	Added initialization calls for DataWatchpointTrace and benchmarking, minor formatting fixes
Inc/HALAL/HALAL.hpp	Added includes for new benchmarking modules
Inc/HALAL/Benchmarking_toolkit/DataWatchpointTrace/DataWatchpointTrace.hpp	Refactored DWT interface with enable/disable methods and overflow detection
Inc/HALAL/Benchmarking_toolkit/Benchmarking.hpp	New file implementing benchmarking macros and runtime measurement infrastructure
CMakeLists.txt	Added RTT source files and include directories to build system
Middlewares/Third_Party/RTT/*	Third-party SEGGER RTT library files (multiple new files)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-27T12:46:25Z

Inc/HALAL/Benchmarking_toolkit/Benchmarking.hpp

+    template<uint32_t Id>
+    __attribute__((always_inline)) inline static void __benchmark_start__(){
+        if constexpr(benchmark_type == BenchmarkType::GDB){
+            __asm__ volatile("BKPT \n");
+        } else if constexpr(benchmark_type == BenchmarkType::RUNTIME){
+            uint32_t cycles = DWT->CYCCNT;
+            if constexpr( logging_type & _RTT){
+                timestamp.id = Id;
+                timestamp.cycles = cycles;
+                SEGGER_RTT_Write(0,&timestamp,sizeof(TimeStamp));
+            }
+        }
+    }
+    template<uint32_t Id>
+    __attribute__((always_inline)) inline static void __benchmark_end__(){
+        if constexpr(benchmark_type == BenchmarkType::GDB){
+            __asm__ volatile("BKPT \n");
+        } else if constexpr(benchmark_type == BenchmarkType::RUNTIME){
+            uint32_t cycles = DWT->CYCCNT;
+            if constexpr( logging_type & _RTT){
+                timestamp.id = Id;
+                timestamp.cycles = cycles;
+                SEGGER_RTT_Write(0,&timestamp,sizeof(TimeStamp));
+            }
+        }
+    }


The benchmark_start and benchmark_end functions read DWT->CYCCNT without any mechanism to handle or account for counter overflows. The num_overflows variable is declared in DataWatchpointTrace but never incremented. When the 32-bit counter wraps around, timing measurements will be incorrect. Consider implementing overflow detection and handling.

The overflow logic is implemented on the host side, to make the target measuremetns as lightweight as possible

Src/HALAL/HALAL.cpp

Copilot · 2025-12-27T12:46:26Z

Inc/HALAL/Benchmarking_toolkit/DataWatchpointTrace/DataWatchpointTrace.hpp

+    static void enable() {
+        DWT->CTRL |= DWT_CTRL_CYCCNTENA | DWT_CTRL_EXCTRCENA_Msk | DWT_CTRL_EXCEVTENA_Msk;  // enables the counter
+        DWT->FUNCTION0 |= (1 << 7) | 0b1000; // enable comparison
+        DWT->COMP0 = 0xFFFFFFFF; // detect overflow
    }


The enable() function modifies DWT hardware registers but does not verify that CoreDebug->DEMCR has the TRCENA bit set, which is required before accessing DWT registers. This could cause the DWT operations to fail silently. Consider adding CoreDebug->DEMCR |= DEMCR_TRCENA before accessing DWT registers.

Copilot · 2025-12-27T12:46:26Z

Inc/HALAL/Benchmarking_toolkit/DataWatchpointTrace/DataWatchpointTrace.hpp

-        DWT->CTRL |= DWT_CTRL_CYCCNTENA;  // enables the counter
-        DWT->CYCCNT = 0;
-        return DWT->CYCCNT;
+    static uint32_t num_overflows;


The static member variable num_overflows is declared but never defined or initialized. This will cause a linker error. Add a definition in the corresponding .cpp file (e.g., uint32_t DataWatchpointTrace::num_overflows = 0;) or make it inline if using C++17 or later.

Suggested change

static uint32_t num_overflows;

inline static uint32_t num_overflows = 0;

Copilot · 2025-12-27T12:46:27Z

Inc/HALAL/Benchmarking_toolkit/DataWatchpointTrace/DataWatchpointTrace.hpp

    static void reset_cnt() {
-        CoreDebug->DEMCR |= DEMCR_TRCENA;
        DWT->CYCCNT = 0;  // reset the counter
-        DWT->CTRL = 0;
-    }

-    static void unlock_dwt() {  // unlock the dwt
-        uint32_t lsr = DWT->LSR;
-        if ((lsr & DWT_LSR_Present_Msk) != 0) {
-            if ((lsr & DWT_LSR_Access_Msk) != 0) {
-                DWT->LAR = DWT_LAR_KEY;
-            }
-        }
    }


The reset_cnt() private method is declared but has an empty function body, and it no longer performs its intended reset operation. This appears to be dead code that should either be removed or properly implemented to reset the counter as its name suggests.

Copilot · 2025-12-27T12:46:27Z

Inc/HALAL/Benchmarking_toolkit/Benchmarking.hpp

+    Benchmarker(){
+        DWT->CTRL |= 1;
+        DWT->CYCCNT = 0;
+        if constexpr(benchmark_type == BenchmarkType::RUNTIME){
+            SEGGER_RTT_Init();
+        }
+    }


The Benchmarker constructor initializes DWT registers (DWT->CTRL and DWT->CYCCNT) without checking if the trace infrastructure is unlocked or enabled via CoreDebug->DEMCR. This could fail silently on some targets. The initialization should call CoreDebug->DEMCR |= DEMCR_TRCENA first, and potentially unlock DWT if needed.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

victor-Lopez25 · 2025-12-28T19:13:13Z

Inc/HALAL/Benchmarking_toolkit/Benchmarking.hpp

+constexpr BenchmarkType benchmark_type = 
+#ifdef RUNTIME_BENCHMARK
+    BenchmarkType::RUNTIME;
+    static const char BENCHMARK_TYPE_RUNTIME[] __attribute__((used)) = "BENCHMARK_RUNTIME";
+#elif defined(GDB_BENCHMARK)
+    BenchmarkType::GDB;
+    static const char BENCHMARK_TYPE_GDB[] __attribute__((used)) = "BENCHMARK_GDB";
+#else 
+    BenchmarkType::NONE;
+    static const char BENCHMARK_TYPE_NONE[] __attribute__((used)) = "BENCHMARK_NONE";
+#endif 


please define benchmark_type in each scope of the #if clause:

#ifdef RUNTIME_BENCHMARK constexpr BenchmarkType benchmark_type = BenchmarkType::RUNTIME; static const char BENCHMARK_TYPE_RUNTIME[] __attribute__((used)) = "BENCHMARK_RUNTIME"; #elif defined(GDB_BENCHMARK) constexpr BenchmarkType benchmark_type = BenchmarkType::GDB; static const char BENCHMARK_TYPE_GDB[] __attribute__((used)) = "BENCHMARK_GDB"; #else constexpr BenchmarkType benchmark_type = BenchmarkType::NONE; static const char BENCHMARK_TYPE_NONE[] __attribute__((used)) = "BENCHMARK_NONE"; #endif

victor-Lopez25 · 2025-12-28T19:31:53Z

Inc/HALAL/Benchmarking_toolkit/Benchmarking.hpp

+inline constexpr int logging_type = 
+#if defined(SD)
+    _SD |
+#else
+    _NONE |
+#endif
+#if defined(RUNTIME_BENCHMARK)
+    _RTT;
+#else
+    _NONE;
+#endif


I know it looks cool but it doesn't look very clear, it could be better to write it like this:

#if defined(RUNTIME_BENCHMARK) #define RUNTIME_BENCHMARK_TAG _RTT #else #define RUNTIME_BENCHMARK_TAG _NONE #endif #if defined(SD) inline constexpr int logging_type = _SD | RUNTIME_BENCHMARK_TAG: #else inline constexpr int logging_type = _NONE | RUNTIME_BENCHMARK_TAG; #endif

or alternatively:

#if defined(RUNTIME_BENCHMARK) #define RUNTIME_BENCHMARK_TAG _RTT #else #define RUNTIME_BENCHMARK_TAG _NONE #endif #if defined(SD) #define BENCHMARK_LOGGING_TYPE _SD | RUNTIME_BENCHMARK_TAG #else #define BENCHMARK_LOGGING_TYPE _NONE | RUNTIME_BENCHMARK_TAG #endif inline constexpr int logging_type = BENCHMARK_LOGGING_TYPE:

or...:

#if defined(RUNTIME_BENCHMARK) #define RUNTIME_BENCHMARK_TAG _RTT #else #define RUNTIME_BENCHMARK_TAG _NONE #endif #if defined(SD) #define SD_BENCHMARK_TAG _SD #else #define SD_BENCHMARK_TAG _NONE #endif inline constexpr int logging_type = RUNTIME_BENCHMARK_TAG | SD_BENCHMARK_TAG:

g0nz4I0 added 12 commits September 11, 2025 14:25

feat/add SEGGER_RTT related files to CMake

5fb8ac6

feat/add SEGGER_RRT files to project

4b954c9

feat/include bencharking toolkit into HALAL.hpp

0540347

fix/remove unused functionality, and clarify usage, (idk how it was w…

5b6578c

…orking beforehand, take a look at the old reset_cnt disabling DWT_CTRL_CYCCNTENA by setting DWT->CTRL to 0)

feat/enable DWT in STLIB

87db87e

feat/benchmarking macros, functions, types

fba47ce

feat: add name into benchmark section

01f6643

fix: make RTT block discoverable

64af0db

add configuration of SEGGER_RTT and benchmarking baseline, also added…

c6fc04c

… overflow check in STLIB::update()

add Frequency measurement, cleanup some code, add global symbols for …

258e4fc

…Benchmarking as well

fix benchmarking packages and calculations

e334132

compile guards for Benchmarking

6ff6ccb

jorgesg82 previously approved these changes Oct 25, 2025

View reviewed changes

jorgesg82 assigned g0nz4I0 Nov 15, 2025

Merge branch 'development' into Benchmarking

b8a0a06

g0nz4I0 dismissed jorgesg82’s stale review via b8a0a06 December 8, 2025 22:01

fix: build

bc89164

g0nz4I0 marked this pull request as draft December 16, 2025 15:41

feat: add header for benchmarks

73fa4a2

Copilot AI review requested due to automatic review settings December 27, 2025 12:44

Copilot started reviewing on behalf of g0nz4I0 December 27, 2025 12:44 View session

Copilot AI reviewed Dec 27, 2025

View reviewed changes

g0nz4I0 and others added 2 commits December 27, 2025 13:47

Update Src/HALAL/HALAL.cpp

7fb1db1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge branch 'development' into Benchmarking

ccb32a0

victor-Lopez25 reviewed Dec 28, 2025

View reviewed changes

fix: address PR review comments

8b9892c

	static uint32_t num_overflows;
	inline static uint32_t num_overflows = 0;

Benchmarking #526

Are you sure you want to change the base?

Benchmarking #526

Uh oh!

Conversation

g0nz4I0 commented Sep 26, 2025

Uh oh!

jorgesg82 commented Dec 14, 2025

Uh oh!

jorgesg82 commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

g0nz4I0 commented Dec 16, 2025

Uh oh!

victor-Lopez25 commented Dec 26, 2025

Uh oh!

g0nz4I0 commented Dec 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

g0nz4I0 Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

victor-Lopez25 Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

victor-Lopez25 Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

victor-Lopez25 Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jorgesg82 commented Dec 14, 2025 •

edited

Loading