boards: kconfig: Fix enabling USE_SEGGER_RTT for various boards

[ulfalizer]

boards: kconfig: Fix enabling USE_SEGGER_RTT for various boards

HAS_SEGGER_RTT is assigned by various Nordic boards, but assignments
have no effect on promptless symbols. This symbol is enabled through
being select'ed by SOC_SERIES_NRF52X.

Holyiot, nRF52833-PCA10100, nRF52840-PCA10056, and nRF52-PCA10040 only
assign HAS_SEGGER_RTT without assigning USE_SEGGER_RTT. They probably
meant to enable USE_SEGGER_RTT, so do that instead.

Also add a help text to HAS_SEGGER_RTT and a warning re. HAS_SEGGER_RTT
vs. USE_SEGGER_RTT to the documentation.

Flagged by #20742.

[henrikbrixandersen]

This should probable be CONFIG_USE_SEGGER_RTT=y since this board has no physical pins for a UART for console.

[ulfalizer]

Fixed. See new PR comment.

[ioannisg]

This does not look like accidental enabling of prompt-less symbols; seems there has been a confusion among the board contributors regarding which symbol actually enables RTT.
Removing these is of course a no-op; i wonder, though, if we could replace these with the correct _USE_RTT symbol.

And since you're touching the documentation, I'd suggest to add like saying that HAS_RTT denotes platform capabilities, while _USE_RTT is the user option to use the feature..

[ulfalizer]

Yeah... I wasn't sure HAS_SEGGER_RTT was even being enabled properly for these boards, so I got a bit lazy while batch-fixing.

Turns out it is, because these are all SOC_SERIES_NRF52X, which has select HAS_SEGGER_RTT.

Did a proper fix that switches the boards that only assigned HAS_SEGGER_RTT to assign USE_SEGGER_RTT instead. Added a help text and a warning too.

See updated commit message.

[lemrey]

One thing is to clean up the assignment of HAS_SEGGER_RTT, another is to change that assignment to USE_SEGGER_RTT; that will change the default board configuration, right? I just built a sample for nrf52840_pca10056 and USE_SEGGER_RTT is n, this patch would change that to y afaics?

[ulfalizer]

One thing is to clean up the assignment of HAS_SEGGER_RTT, another is to change that assignment to USE_SEGGER_RTT; that will change the default board configuration, right? I just built a sample for nrf52840_pca10056 and USE_SEGGER_RTT is n, this patch would change that to y afaics?

Yup... we're assuming people wanted to enable it, but assigned the wrong symbol. Could be wrong.

[carlescufi]

I am not quite convinced by this, because Segger RTT support is determined at the board level, not at the SoC level, since RTT is a mechanism that is implemented in the debugger chip that sits on the board, not in the SoC itself. The original idea with this was to set HAS_SEGGER_RTT at the board level to indicate which boards support this, but it might now have gone out of date. Perhaps @anangl or @nordic-krch could comment as well.

[lemrey]

Yup... we're assuming people wanted to enable it, but assigned the wrong symbol. Could be wrong.

Personally, I'd leave the default setting.

Plus I agree with what @carlescufi said, HAS_SEGGER_RTT is a board capability. For example the nrf52840 SoC is present on two boards pca10056 and pca10059 the former has Segger the latter doesn't. We should have HAS_SEGGER_RTT in the board defconfig files.

[ioannisg]

I am fine with any solution you pick, I would only like to see us having 2 Kconfig options - one _HAS_RTT to signify RTT capabilities (at SoC or Board level), and a user option (depending on that) for enabling the feature.

[ulfalizer]

Yup... we're assuming people wanted to enable it, but assigned the wrong symbol. Could be wrong.

Personally, I'd leave the default setting.

Plus I agree with what @carlescufi said, HAS_SEGGER_RTT is a board capability. For example the nrf52840 SoC is present on two boards pca10056 and pca10059 the former has Segger the latter doesn't. We should have HAS_SEGGER_RTT in the board defconfig files.

So do you think HAS_SEGGER_RTT should be user-configurable?

Note that assigning it is currently a no-op, because it isn't user-configurable (since it has no prompt).

One icky thing about having it user-configurable is that you could go into the menuconfig and toggle it, even though it's a fixed hardware thing (afaik).

[ulfalizer]

I am not quite convinced by this, because Segger RTT support is determined at the board level, not at the SoC level, since RTT is a mechanism that is implemented in the debugger chip that sits on the board, not in the SoC itself. The original idea with this was to set HAS_SEGGER_RTT at the board level to indicate which boards support this, but it might now have gone out of date. Perhaps @anangl or @nordic-krch could comment as well.

Could select HAS_SEGGER_RTT from the board symbols instead, if that'd make more sense. Not sure if I'm the most qualified guy to do it though.

Maybe we could just go with something that works as intended here.

[ulfalizer]

Depends on #21961.

[carlescufi]

I am not quite convinced by this, because Segger RTT support is determined at the board level, not at the SoC level, since RTT is a mechanism that is implemented in the debugger chip that sits on the board, not in the SoC itself. The original idea with this was to set HAS_SEGGER_RTT at the board level to indicate which boards support this, but it might now have gone out of date. Perhaps @anangl or @nordic-krch could comment as well.

Could select HAS_SEGGER_RTT from the board symbols instead, if that'd make more sense. Not sure if I'm the most qualified guy to do it though.

Maybe we could just go with something that works as intended here.

I don't think HAS_SEGGER_RTT should be user-configurable. It should be just like any other board-specific setting that is hardcoded on the board defconfig, since it's essentially a "has" property, just like CPU_HAS_FPU. It's a bit trickier because people can indeed override the firmware in the board's debug controller and get rid of Segger, but I still think this makes sense as a non-configurable property of the board. The one that should be configurable is USE_SEGGER_RTT.

[ulfalizer]

#21961 is in now, so removing DNM.

I don't think HAS_SEGGER_RTT should be user-configurable. It should be just like any other board-specific setting that is hardcoded on the board defconfig, since it's essentially a "has" property, just like CPU_HAS_FPU. It's a bit trickier because people can indeed override the firmware in the board's debug controller and get rid of Segger, but I still think this makes sense as a non-configurable property of the board. The one that should be configurable is USE_SEGGER_RTT.

USE_SEGGER_RTT is the only one that's configurable. The issue was that configuration files tried to enable HAS_SEGGER_RTT.

Question is if the configuration files that enabled HAS_SEGGER_RTT (in a no-op way) really meant to enable USE_SEGGER_RTT. Latest version of the PR is assuming that.

Maybe I'm missing something. Will look more tomorrow.

[ulfalizer]

@lemrey

Personally, I'd leave the default setting.

Plus I agree with what @carlescufi said, HAS_SEGGER_RTT is a board capability. For example the nrf52840 SoC is present on two boards pca10056 and pca10059 the former has Segger the latter doesn't. We should have HAS_SEGGER_RTT in the board defconfig files.

As in just removing the (no-op) CONFIG_HAS_SEGGER_RTT=y assignments, instead of changing them to CONFIG_USE_SEGGER_RTT=y?

Note that all the CONFIG_HAS_SEGGER_RTT=y assignments had no effect previously, and that HAS_SEGGER_RTT is already enabled for these boards (through select).