Skip to content
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

Jump to bottom

sys: add dbgpin module for debugging and profiling (take 2) #15931

Merged
merged 10 commits into from Mar 9, 2021
Merged

sys: add dbgpin module for debugging and profiling (take 2) #15931

merged 10 commits into from Mar 9, 2021

Conversation

haukepetersen
Copy link
Contributor

Contribution description

Successor of #14304

This PR contains a slightly reworked version of the code PRed in #14304:

  • this module is now a header file + some Makefile entries only. Thanks @maribu for the pointers!
  • the usage of this module now depends on the dbgpin feature to be provided
  • This PR only contains the feature and initialization hook for cortexm platforms
  • all other platforms can/should be added on demand step by step, this way we don't end up in this huge loop once more...

With these changes the PR is much slimmer now.

Testing procedure

Run the tests/dbgpin example on any cortexm board. Either check the output of the default pin (GPIO_PIN(0,0)) or define alternative pin(s). The output is best observed with a scope o logic analyzer...

Issues/PRs references

successor of #14304

@haukepetersen haukepetersen changed the title Add dbgpin3 sys: add dbgpin module for debugging and profiling (take 2) Feb 4, 2021
@haukepetersen haukepetersen mentioned this pull request Feb 4, 2021
Closed
miri64
miri64 reviewed Feb 4, 2021
View reviewed changes
sys/include/dbgpin.h Outdated Show resolved Hide resolved
@fjmolinas
Copy link
Contributor

Since in #14304 I had already tested AVR and msp430, mind adding those as well (or in follows up), especially since from what I see the differences seem to be almost none (could you point them out to me if there are any important :))

@haukepetersen
Copy link
Contributor Author

@fjmolinas Sure! I was thinking about that, but then wasn't there still an open question with regards to the position of the dbgpin_init call for the msp?

@haukepetersen
Copy link
Contributor Author

Done:

  • readded msp430 and atmega support
  • remove leftover external var definition (accidentally squashed)

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 5, 2021
@fjmolinas
Copy link
Contributor

@fjmolinas Sure! I was thinking about that, but then wasn't there still an open question with regards to the position of the dbgpin_init call for the msp?

From the PR we had mentioned that fixing the pin initialization for msp430 was out of scope, but do as you prefer.

maribu
maribu reviewed Feb 5, 2021
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

looks good to me, some style suggestions - feel free to ignore

sys/include/dbgpin.h Show resolved Hide resolved
tests/dbgpin/main.c Outdated Show resolved Hide resolved
tests/dbgpin/main.c Outdated Show resolved Hide resolved
tests/dbgpin/main.c Outdated Show resolved Hide resolved
tests/dbgpin/main.c Outdated Show resolved Hide resolved
tests/dbgpin/main.c Outdated Show resolved Hide resolved
tests/dbgpin/main.c Outdated Show resolved Hide resolved
tests/dbgpin/main.c Outdated Show resolved Hide resolved
@haukepetersen
Copy link
Contributor Author

@maribu thanks for the feedback, improved the test app by using xtimer_msleep() as suggested. I am using the xtimer now since it was introduced many years ago, but for some reason I missed the existence of that function :-)

@maribu
Copy link
Member

maribu commented Feb 5, 2021

xtimer_msleep() has been introduced quite recently only. The main motivation was allowing use of ZTIMER_MSEC in ztimer_xtimer_compat to directly profit from the power savings, without having to run a cocinelle script first :-)

@haukepetersen
Copy link
Contributor Author

Very nice indeed!

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Feb 5, 2021
@haukepetersen haukepetersen removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 15, 2021
@haukepetersen
Copy link
Contributor Author

The kconfig issues should be fixed now.´ However, there is another issue with some modules, that define the _DEFAULT_SOURCE flag for to gain access to certain newlib functions (e.g. devfs used be the mulle board). I have a fix for this, but will open a separate PR for that to keep things tidy...

@haukepetersen haukepetersen mentioned this pull request Feb 15, 2021
Closed
@haukepetersen
Copy link
Contributor Author

Fix is proposed in #16010

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 15, 2021
@haukepetersen
Copy link
Contributor Author

I thought I speed this up a little: I removed including the dbgpin.h header globally. Now one has to include that header manually when using any dbgpin function in a file, but for this we get rid of any header clashes and we do not need to wait for #16010. After we figured out a solid solution for #16010 we can then think about re-adding the global inclusion of the dbgpin.h header. Sounds fair?!

@haukepetersen haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 26, 2021
@haukepetersen
sys: initial import of dbgpin module
2d582cd
@haukepetersen
tests: initial import of dgbpin test
84660ed
@haukepetersen
cpu/cortexm_common: add dbgpin initialization
47a031e
@haukepetersen
cpu/atmega_common: add dbgpin initialization
71e9a9e
@haukepetersen
cpu/msp430_common: add dbgpin initialization
717a125
@haukepetersen
cpu/kinetis: move IRQ name adaption to cpu_conf.h
edb890f 
Found and fixed the issue for the kinetis-based boards: The kinetis
code is using some macros to map some IRQ names, that differ in
some versions of vendor headers, to a RIOT wide unique name. The
doxygen of this mapping states, that this mapping must be done before
any vendor header is included. Unfortunately, the mapping was so far
placed in cpu/kinetis/vectors.c, before any other include statement.

In some cases, the vendor headers might be included before the
mapping macros in vectors.c, leading to the compilation errors down
the line. To fix this, the adaption defines are moved into
cpu/kinetis/cpu_conf.h, which is the file that actually includes
the vendor headers. This way it is ensured, that these adaption
macros are always defined before any vendor header is included,
and therefore preventing this kind of error for good.
@haukepetersen
kconfig: add dbgpin feature
984edc1
@haukepetersen
cpu/cortexm_common/kconfig: add dbgpin feature
899fe63
@haukepetersen
cpu/atmega_common/kconfig: add dbgpin feature
91f9d7d
@haukepetersen
cpu/msp430_common/kconfig: add dbgpin feature
ccca985
@haukepetersen
Copy link
Contributor Author

after removing the global include I forgot to explicitly include the dbgpin header in the platform specific initialization files the call dbgpin_init() -> this is now fixed and hopefully Murdock should be happy now.

@fjmolinas
Copy link
Contributor

Unrelated failure because of a non-update worker, triggering CI again.

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 1, 2021
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 1, 2021
@fjmolinas
Copy link
Contributor

all green go!

@fjmolinas fjmolinas merged commit fc82e39 into RIOT-OS:master Mar 9, 2021
@nandojve nandojve mentioned this pull request Mar 9, 2021
Merged
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@maribu maribu maribu approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

@gschorcht gschorcht Awaiting requested review from gschorcht gschorcht is a code owner

@kYc0o kYc0o Awaiting requested review from kYc0o kYc0o is a code owner

@roberthartung roberthartung Awaiting requested review from roberthartung

@cgundogan cgundogan Awaiting requested review from cgundogan

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees
No one assigned
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2021.04
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@haukepetersen @fjmolinas @maribu @miri64 @benpicco @kaspar030