Skip to content

Final prerequisites to improve debugger output #454

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.

Sign up for GitHub

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

Merged
hathach merged 10 commits into from
Mar 20, 2020
Merged

Final prerequisites to improve debugger output #454

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
64b05d3
Fix stack trace debug level check
henrygab Mar 7, 2020
4565d0c
Use uppercase PRINTF.
henrygab Mar 7, 2020
decde55
Use `Print` class reference
henrygab Mar 8, 2020
b8fb0cd
Add locations for two additional defines.
henrygab Mar 8, 2020
03805f3
Define PRINTF macro regardless of debug level.
henrygab Mar 11, 2020
7777bd9
Enable CI
henrygab Mar 11, 2020
47d1b4d
Add defaults to global platforms.txt
henrygab Mar 11, 2020
7fab012
Undo accidental change
henrygab Mar 11, 2020
f532a27
And disable Travis-CI once again.
henrygab Mar 11, 2020
2709813
More printf() --> PRINTF() conversion
henrygab Mar 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add defaults to global platforms.txt
  • Loading branch information
@henrygab
henrygab committed Mar 11, 2020
commit 47d1b4d23f33c537e6d9a6fe38b227053cfce3d6
3 changes: 3 additions & 0 deletions platform.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ compiler.size.cmd=arm-none-eabi-size
# this can be overriden in boards.txt
build.float_flags=-mfloat-abi=hard -mfpu=fpv4-sp-d16 -u _printf_float
build.debug_flags=-DCFG_DEBUG=0
build.logger_flags=-DCFG_LOGGER=1
Copy link
Member

@hathach hathach Mar 19, 2020

Choose a reason for hiding this comment

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

@henrygab can we skip to include CFG_LOGGER and CFG_SYSVIEW in the platforms.txt. Let's just enable them based on teh CFG_DEBUG value. If you want to enable it in other scenario, just modify the source directly. Would that make sense ??

Copy link
Collaborator Author

@henrygab henrygab Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @hathach,
Thank you for the review.

Two questions, each with their own answer. The short answer is that editing the source files in the BSP does not make sense in many scenarios. Let me address each in turn...

[Click to expand] As to CFG_LOGGER...

Summary good news: As requested, I have no plans to update the boards.txt file to override these in the BSP. As you indicate, the value can be updated by advanced users by editing the source code of the BSP.

At the same time, it is desirable to ensure Arduino-standard extensibility points remain open, especially when they are particularly useful in classroom settings. Let me explain...

[Click to expand] Extensibility and why the default value was added

As you know, Arduino allows a overriding settings via a boards.local.txt file. This is an extremely useful setting for a teacher / IT staff to enable custom menu options, although I understand that you yourself would never use this file. This functionality is an Arduino standard, and part of the flexibility that the Arduino ecosystem provides -- IT staff / teachers can provide a customized boards.local.txt to allow students to:

  1. Use the Board Manager in the Arduino UI without restriction, and
  2. Simultaneously add menu options to the Arduino UI that customize an existing board

Here, the long-term goal is to enable choice of alternative (e.g., higher-speed, lower-overhead) debug logging via this extensibility mechanism ... Critically, without editing the BSP files. After all, having a classroom use git (instead of the Board Manager) is not a desirable outcome ... it just adds too many layers of complexity when the goal is to teach coding. At the same time, they may need both Serial ports for their project, and thus need to redirect the debug output to an LCD, to a file, etc.

While the entire line could technically be duplicated in an IT-provided platform.local.txt, the maintenance complexity is directly proportional to the length of the lines being added. The build.flags.nrf setting line is one of the longest, and thus it is preferable to lay the groundwork to provide an alternative. (Yes, I know you don't use this feature.)

I initially simply attempted to add the variables {build.logger_flags} {build.sysview_flags} in , without providing a default value. Really. It's in the commit history.

Unfortunately, without a default value, compilation fails. Therefore, a default value was necessary to avoid encumbering the Arduino .local.txt feature.

Because having a default value does not cause any compilation or maintenance problem, and because I am supporting your request not to add these options to the BSP's boards.txt list of options, this has no effect. Thus, I added this commit, not because I forgot or ignored your earlier request to use the sources files (rather than the BSP's boards.txt file), but as a minimum necessary compromise to achieve your request while ensuring a smooth path forward for the goal of enabling alternate debug outputs.

BUT DO NOT FEAR...

Finally, I want to assure you that, although I had to add the default value into platforms.txt to allow this to build, I have every intention of continuing to support your request to NOT change the BSP's boards.txt -- I will not add the extra debug menu items there unless you request it, which is in line with your earlier feedback.

[Click to expand] As to CFG_SYSVIEW...

I'm glad you understand that enabling of SYSVIEW may be desirable, without also enabling the verbose logging that a debug level of 3+ causes. Normal debug level of 3+ simply causes too much overhead, hides timing-related bugs, and prevents use of SYSVIEW for any performance analysis.

As you can see, this PR does not change the existing behavior:

  • CFG_SYSVIEW is disabled by default
  • boards.txt options enable SYSVIEW when debug level is set to 3+

As noted above with respect to CFG_LOGGER, this PR lays the groundwork to allow this advanced functionality to be enabled, without requiring the user to edit BSP files. This is important in classrooms and maker spaces, and even for online tutuorials. (again, see more comprehensive explanation in prior section on the Arduino extensibility points).

Therefore, please accept this small addition, as it is necessary to simplify use of the standard Arduino extensibility points.

The Adafruit Feather nRF52840 Express is the only Feather with the built-in 10-pin debug header, and so is uniquely positioned for advanced classes, and provides a wonderful bridge when advancing users from Arduino to more advanced environments { platform.io, Segger Embedded Studio }. I like this board.

The PR has zero functional change, has zero GUI changes (no menu options), and simultaneously makes forward progress to enable Arduino extensibility points for advanced class use.

Furthermore, I am actively ensuring your prior (and current) concerns are being addressed. I have no plan for the next PRs to add any menu items, or to otherwise change the default logger of the BSP.

Please reconsider!

Copy link
Member

@hathach hathach Mar 19, 2020

Choose a reason for hiding this comment

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

@henrygab thanks for detail explanation, I am not aware of the use case of board/platforms.local.txt in the class/workshop as you mentioned. That is indeed a good scenario. Can I have my last attempt to ask you to bundle the build.logger_flags + build.sysview_flags into 1 flag, maybe build.rtt_flags, build.extra_debug_flags, or anything that makes sense to you e.g

build.extra_debug_flags=-DCFG_DEBUG=0 -DCFG_SYSVIEW=0

build.sysview_flags=-DCFG_SYSVIEW=0

# common compiler for nrf
rtos.path={build.core.path}/freertos
Expand Down Expand Up @@ -148,3 +150,4 @@ tools.bootburn.bootloader.pattern={program.burn_pattern}
tools.bootburn.erase.params.verbose=
tools.bootburn.erase.params.quiet=
tools.bootburn.erase.pattern=