Segger RTT: support CONFIG_SEGGER_RTT_INIT_MODE and optimizations #17

giansta · 2023-01-06T20:19:30Z

Different initialization modes of Control Block
Safer INIT() like in SystemView V352a
Made _aTerminalId const like in SystemView V352a

Fixes #53544.

giansta · 2023-01-06T21:04:30Z

Expected to fix Issue #53544 with PR #53569 in Zephyr repo

nordic-krch · 2023-03-10T08:21:33Z

SEGGER/SEGGER_RTT.c

@@ -167,6 +167,26 @@ Additional information:
  #endif
 #endif

+#ifndef   SEGGER_RTT_MEMCMP


is this a new segger version or your modifications?
I'm afraid that those changes may get lost if someone updates with new version of segger_rtt.

It's my modifications. I was assuming that Segger people follow this repo and approve these changes so that they become they also take them.
If not, what do you suggest? Contact them through their official website?

When I look in previous commits, they don't seem to be done by Segger, like my one. Are there some additional rules to coordinate with Segger?

I'm not sure how to push it to segger.

Maybe @sigvartmh or @carlescufi can help?

Segger is not part of the zephyrproject-rtos Git-Repo.
However, we will review the changes and extend the official RTT sources if we
come to the conclusion that it is a good addition to the current RTT state.
I will keep you posted.

I got this reply after opening a ticket in Segger support.

After opening a ticket in support.segger.com, they proposed the update V7.86h that gives equivalent changes and improved initialization. I tried to leave the SEGGER_RTT.c untouched and follow their suggestion to not call at all SEGGER_RTT_Init() in the chained application, but, even if working, I saw that the APIs using use INIT() aren't used in Zephyr log backend. So I added a change only in SEGGER_RTT_Init(), using the configuration I'm aware of that distinguish the cases when we want to to a forced init of CB or just check if it's already done.

nordic-krch · 2023-03-15T11:09:50Z

SEGGER/SEGGER_RTT.c

+
+#ifndef   SEGGER_RTT_MEMSET
+  #ifdef  MEMSET
+    #define SEGGER_RTT_MEMSET(pDest, FillChar, NumBytes)  MEMCPY((pDest), (FillChar), (NumBytes))


copy paste error. MEMCPY -> MEMSET

Thanks for spotting this, fixed.

nordic-krch · 2023-03-24T13:19:37Z

SEGGER/SEGGER_RTT.c

@@ -167,6 +167,26 @@ Additional information:
  #endif
 #endif

+#ifndef   SEGGER_RTT_MEMCMP


I'm not sure how to push it to segger.

MarcianoPreciado · 2023-05-09T00:53:51Z

Can we get this reviewed? This is causing me issues also

carlescufi · 2023-05-19T11:33:05Z

@sigvartmh please review this as part of this change
@giansta please rebase since we've updated to RTT 3.40

MarekPieta · 2023-05-23T09:18:34Z

SEGGER/SEGGER_RTT.c

+   * Segger code using _DoInit() is used. For applications started by
+   * bootloader, Segger recommends not calling this function, as all the
+   * recommended APIs use INIT(). Unfortunately Zephyr log backend uses
+   * SEGGER_RTT_WriteSkipNoLock() that doesn't call INT(), so we need


typo: INIT

Maybe we could add a dedicated Kconfig option to let user decide if Zephyr application should actually force overwrite of the RTT structures during initialization. The current approach cannot support e.g. other bootloaders.
That would also allow to preserve RTT content during e.g. software reboot that does not clear RAM content.

I also wonder if it wouldn't be reasonable to check whole acID right before initialization (instead of checking just the first byte) to ensure no false positives in case RAM content is not zero-initialized (with current implementation chance of false positive when checking if structure is initialized is quite high - ~1/255).
Actually after we improve the check, we may event consider to always call INIT here

If we are going to rely on bootloader to initialize the RTT structures, we need to ensure backwards compatibility of the initialization operation (many embedded applications rely on immutable bootloader that cannot be updated once flashed).

Thanks for your suggestions. Unfortunately I was too busy and I'll be out now for a couple of weeks, but afterwards I'll try to follow them.
The Kconfig option is a good idea, I was not very satisfied with my #ifdefs that are less general. I just have to figure out where such a Kconfig can be placed.
I also agree on the whole acID check, the INIT() macro can be used like it is only for other functions than SEGGER_RTT_Init(). I tried not to touch the SEGGER module, then trying with minimal change, but this is indeed reasonable.
I'll even rebase to RTT 3.40.

With latest release I tried to take account of all above suggestions:

Rebased to SystemView V340

Managed dedicated CONFIG_SEGGER_RTT_INIT_MODE to have different init behaviours

The second change gives more flexibility: application level configs can force initialization of control block without condition or have a complete or partial check on acID to have a contitional init. Yet this requires some more changes in Segger code.

The SystemView V340 code misses some minor changes (safer INIT(), const _aTerminalId) tht are provided by latest SystemView V352a: I've added them, as in previous version.

The CONFIG_SEGGER_RTT_INIT_MODE is defined in PR #53569 in Zephyr repo.

Thanks for all your changes looks good :) I've done similar modifications but haven't pushed changes to Zephyr due to having it interop with other parts of downstream Nordic Fork, which I couldn't figure out how to handle in a nice way. However having this upstream would definitely be a good addition.

henrikbrixandersen

I generally think this looks good, but it could benefit from either a more elaborate Git commit message describing the changes in detail, or more code comments - or both.

giansta · 2024-04-26T11:02:11Z

I generally think this looks good, but it could benefit from either a more elaborate Git commit message describing the changes in detail, or more code comments - or both.

I tried to have the commit messages clearer.
With regard to comments, I believe that most code changes are taken from Segger code and they should be quite clear.
The key point is in SEGGER_RTT_Init(), where I wrote a comment explaining the reason of changes.
If you feel that something else could be enhanced, please let me know.

Config/SEGGER_RTT_Conf.h

SEGGER/SEGGER_RTT.c

cfriedt · 2024-07-25T15:24:42Z

SEGGER/SEGGER_RTT.c

+  volatile SEGGER_RTT_CB* p;
+  p = (volatile SEGGER_RTT_CB*)((char*)&_SEGGER_RTT + SEGGER_RTT_UNCACHED_OFF);
+  unsigned i;
+  bool DoInit = false;


I needed to include <stdbool.h> at the top of this file to avoid this error when building against v3.6-branch

SEGGER/SEGGER_RTT.c:1906:3: error: unknown type name 'bool' 1906 | bool DoInit = false;

Thanks @cfriedt, for signalling this issue. It probably depends on the toolchain, as I didn't have it.
Yet, looking at Seger code, they seem not to use boolean, so I changed it to int without using false/true values.

cfriedt · 2024-07-25T15:26:04Z

SEGGER/SEGGER_RTT.c

+  RTT__DMB();                       // Force order of memory accesses for cores that may perform out-of-order memory accesses
+  for (i = 0; i < sizeof(_aInitStr) - 1; ++i) {
+    if (p->acID[i] != _aInitStr[sizeof(_aInitStr) - 2 - i]) { // Skip terminating \0 at the end of the array
+    	DoInit = true;


Similar error / need for <stdbool.h>

SEGGER/SEGGER_RTT.c:1913:16: error: 'true' undeclared (first use in this function) 1913 | DoInit = true;

Support Zephyr "RTT Initialization mode" config choice, allowing Control Block initialization to be retained from another program (e.g. bootloader). It includes conditional CB init with full check on Control Block ID or partial one that checks only the first byte, yet improving its logic. Minor optimizations with static const added. Signed-off-by: Giancarlo Stasi <giancarlo.stasi.co@gmail.com>

Allow to define the name of custom RTT section both when it's configured to be placed at RAM start and when its address is defined by a memory region in DTS. Signed-off-by: Giancarlo Stasi <giancarlo.stasi.co@gmail.com>

henrikbrixandersen

LGTM

This was referenced Jan 6, 2023

debug: tracing: Add Segger RTT linker section options zephyrproject-rtos/zephyr#53569

Merged

Cannot see both bootloader and application RTT output zephyrproject-rtos/zephyr#53544

Closed

giansta force-pushed the fix/rtt-cb-common-addr branch from 0f2d763 to 4d3b32a Compare January 9, 2023 15:35

giansta force-pushed the fix/rtt-cb-common-addr branch from 4d3b32a to 71e53e6 Compare January 30, 2023 21:26

nordic-krch reviewed Mar 10, 2023

View reviewed changes

nordic-krch reviewed Mar 24, 2023

View reviewed changes

giansta force-pushed the fix/rtt-cb-common-addr branch from 71e53e6 to 5c21108 Compare March 27, 2023 14:09

giansta force-pushed the fix/rtt-cb-common-addr branch from 5c21108 to 76a9690 Compare April 14, 2023 13:27

giansta changed the title ~~Zero-init RTT control block fields if not in BSS, don't init it twice~~ Zero-init RTT control block if not in BSS, don't init it twice Apr 14, 2023

MarekPieta suggested changes May 23, 2023

View reviewed changes

giansta force-pushed the fix/rtt-cb-common-addr branch from fc79836 to 7194d9b Compare August 29, 2023 22:14

giansta changed the title ~~Zero-init RTT control block if not in BSS, don't init it twice~~ Segger RTT: support CONFIG_SEGGER_RTT_INIT_MODE and optimizations Aug 29, 2023

giansta force-pushed the fix/rtt-cb-common-addr branch from 44e7b52 to 0a02f8b Compare April 17, 2024 10:27

nordicjm requested review from nordic-krch and MarekPieta April 17, 2024 10:31

sigvartmh approved these changes Apr 17, 2024

View reviewed changes

henrikbrixandersen reviewed Apr 25, 2024

View reviewed changes

giansta force-pushed the fix/rtt-cb-common-addr branch from 0a02f8b to 0ff9539 Compare April 26, 2024 10:56

henrikbrixandersen requested changes Apr 28, 2024

View reviewed changes

Config/SEGGER_RTT_Conf.h Show resolved Hide resolved

SEGGER/SEGGER_RTT.c Show resolved Hide resolved

SEGGER/SEGGER_RTT.c Show resolved Hide resolved

cfriedt reviewed Jul 25, 2024

View reviewed changes

giansta added 2 commits August 11, 2024 17:32

giansta force-pushed the fix/rtt-cb-common-addr branch from 0ff9539 to fe8fb43 Compare August 11, 2024 15:34

nordicjm requested review from cfriedt and henrikbrixandersen August 12, 2024 10:24

henrikbrixandersen approved these changes Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Segger RTT: support CONFIG_SEGGER_RTT_INIT_MODE and optimizations #17

Segger RTT: support CONFIG_SEGGER_RTT_INIT_MODE and optimizations #17

giansta commented Jan 6, 2023 •

edited

Loading

giansta commented Jan 6, 2023

nordic-krch Mar 10, 2023

giansta Mar 10, 2023

giansta Mar 13, 2023

nordic-krch Mar 24, 2023

giansta Mar 27, 2023

giansta Apr 5, 2023

giansta Apr 14, 2023

nordic-krch Mar 15, 2023

giansta Mar 27, 2023

nordic-krch Mar 24, 2023

MarcianoPreciado commented May 9, 2023

carlescufi commented May 19, 2023

MarekPieta May 23, 2023

MarekPieta May 23, 2023

MarekPieta May 23, 2023 •

edited

Loading

MarekPieta May 23, 2023

giansta Jun 9, 2023 •

edited

Loading

giansta Aug 29, 2023 •

edited

Loading

sigvartmh Apr 17, 2024

henrikbrixandersen left a comment

giansta commented Apr 26, 2024

cfriedt Jul 25, 2024 •

edited

Loading

giansta Aug 12, 2024 •

edited

Loading

cfriedt Jul 25, 2024

giansta Aug 12, 2024

henrikbrixandersen left a comment

Segger RTT: support CONFIG_SEGGER_RTT_INIT_MODE and optimizations #17

Are you sure you want to change the base?

Segger RTT: support CONFIG_SEGGER_RTT_INIT_MODE and optimizations #17

Conversation

giansta commented Jan 6, 2023 • edited Loading

giansta commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcianoPreciado commented May 9, 2023

carlescufi commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarekPieta May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giansta Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

giansta Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikbrixandersen left a comment

Choose a reason for hiding this comment

giansta commented Apr 26, 2024

cfriedt Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

giansta Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikbrixandersen left a comment

Choose a reason for hiding this comment

giansta commented Jan 6, 2023 •

edited

Loading

MarekPieta May 23, 2023 •

edited

Loading

giansta Jun 9, 2023 •

edited

Loading

giansta Aug 29, 2023 •

edited

Loading

cfriedt Jul 25, 2024 •

edited

Loading

giansta Aug 12, 2024 •

edited

Loading