sys: added pindbg module for direct control of debug pins

[haukepetersen]

The reason that we still have the direct controlled LED macros in RIOT is to have some means for controlling pins with the least possible amount of overhead for debugging and profiling purposes. This PR proposes a separate and optional module for this purpose.

So far, it only works for stm32 based boards. Use e.g. it like this:

USEMODULE+=pindbg CFLAGS="-DPINDBG1=GPIO_PIN\(0,1\) -DPINDBG0=GPIO_PIN\(2,17\)" BOARD=stm32f3discovery make

IMHO it makes sense to require the user of the pindbg module to explicitly define the pins he/she wants to use, as this is very specific to the application run on a board (i.e. which peripherals are use and which pins are actually free for debugging).

Putting this module in place could help with the discussion about cleaning up the LED interfaces (see #7321 and #7350).

[smlng]

what is the rational that this should go into sys rather than drivers?

[haukepetersen]

I think this module is border line. IMHO it is not really a device driver, but rather a system module used for debugging that makes use of the periph/gpio driver... In the end, not everything that uses periph drivers has to be a device driver!?

[haukepetersen]

Not in the code, yet: with this PR we can also very easily define default debug pins on a per-board basis: simply add something like this to the board.h:

#ifndef PINDBG0
#define PINDBG0    GPIO_PIN(PORT_A, 13)
#endif

So as default, we may simply map one or both debug pins to on-board LEDs, and they will behave exactly like the old LED_xx macros. But this way, they are more intuitive to define compared to the old LED macros, and also more easy to re-configure during compile time.

@OlegHahm: would this suffice as alternative to the existing LED macros in your opinion?

[smlng]

IMHO it would be great to make this more generic, that is for arbitrary numbers of debug pins, and make the config consistent with other peripheral confs. So I was thinking about something like:

static const pindbg_cfg_t pindbg_config[] = {
    GPIO_PIN(0, 1),
    GPIO_PIN(0, 2),
}

#define PINDGB_NUMOF ( sizeof(pindbg_config) / sizeof(pindbg_config[0] )

and then also have MCU specific macros like

#define PINDBG_PORT(x)        ((GPIO_TypeDef *)(x & ~(0x0f)))
#define PINDBG_MASK(x)        (1 << (x & 0xf))
#define PINDBG_SET(x)         (PINDBG_PORT(x)->BSRR = PINDBG_MASK(x))
#define PINDBG_CLR(x)         (PINDBG_PORT(x)->BSRR = (PINDBG_MASK(x) << 16))
#define PINDBG_TGL(x)         (PINDBG_PORT(x)->ODR ^= PINDBG_MASK(x))

which then would turn pindbg_init() into a simple and generic for-loop.

@haukepetersen you think that's possible and handy?

[haukepetersen]

With this I see two problems:

• it is not possible to simply redefine a pin during compile time, e.g. DBGPIN0=GPIO_PIN\(0,2\) make ...
• I am not sure, if the set/clr/tgl expressions will still evaluate to a single instruction as they do in the current code

[smlng]

for the first:

#ifndef PINDBG0
#define PINDBG0 GPIO_PIN(0, 1)
#endif
#ifndef PINGDBG1
#define PINDBG0 GPIO_PIN(0, 2)
#endif 

static const pindbg_cfg_t pindbg_config[] = {
    PINDBG0,
    PINDBG1,
}

and for the second: its preprocessor text replacement, i.e, my example above should resolve to the same statement as you static defines where ever it is used in the code.

[kaspar030]

To me "pindbg" as name feels the wrong way, "dbgpin" would make more sense. "pindbg" sounds like a module for debugging the pins. Do I make sense? :)

[smlng]

To me "pindbg" as name feels the wrong way, "dbgpin" would make more sense

yeah was thinking the same, but didn't got myself convinced that the latter variant is really more beautiful or comprehensive.

[kaspar030]

Is it possible to just add "static pin init/set/clear/toggle"-functions/macros to periph/gpio? Having gpio.h, led.h and now pindbg.h seems to me like a statement that our gpio interface doesn't optimize optimally, as long as the latter two don't just wrap gpio.h.

[haukepetersen]

"dbgpin" would make more sense

I agree, will change the name.

Is it possible to just add "static pin init/set/clear/toggle"-functions/macros to periph/gpio?

led.h does exactly that.

seems to me like a statement that our gpio interface doesn't optimize optimally

Optimization is at least not guaranteed -> as I found out using #7478, the gpio_set/clr/toggle are only properly optimized, when building with LTO. But as this is not the default case, I think there is no way around going the 'manual bit shovel way', if we want to guarantee 1-instruction operation, as we want/need for the debug pins.

[haukepetersen]

@smlng will try your proposed solution and verify if the compiler does its job. If yes, I will adapt the code.

[haukepetersen]

also changed the name to dbgpin and moved the module to drivers.

[miri64]

@haukepetersen any plans to work on this further? Otherwise I would close as archived.

[haukepetersen]

yes, I still would like to have this PR merged. Though its hard to find time to work on this lately... I will give my best.

[haukepetersen]

closing in favor of #14304