Hi,
I already put some of comments below in (already merged) PR which seems to
add this functionality, but this gives more context about the change itself
so I have few more comments.
On Fri, May 3, 2019 at 1:57 AM Vipul Rahane <vrah...@gmail.com> wrote:
> Hello,
>
> So, we are running into an issue where some peripheral needs to get shut
> down before the breakpoint gets hit. this only happens when the debugger is
> connected and so, it quite isolated in terms on functionality.
>
It would be better to have such callback/whatever as a general option, not
only while debugger is connected, since this makes it more generic and you
can easily check if debugger is connected in your callback.
> There are different approaches we could follow:
>
> 1. Have a macro (syscfg) define that function and call that macro
>
> 154 #ifdef MYNEWT_VAL_HAL_SYSTEM_PRE_BKPT_CB
> 155 HAL_SYSTEM_PRE_BKPT_CB();
> 156 #endif
> 157
> 158 #if !MYNEWT_VAL(MCU_DEBUG_IGNORE_BKPT)
> 159 asm("bkpt");
> 160 #endif
>
> 2. Have a `hal_system_pre_bkpt_cb()` function, register a callback and have
> one specific MCU call it only if a syscfg is set, something like:
>
> 154 #ifdef MYNEWT_VAL(HAL_SYSTEM_PRE_BKPT_CB)
> 155 hal_system_pre_bkpt_cb();
> 156 #endif
> 157
> 158 #if !MYNEWT_VAL(MCU_DEBUG_IGNORE_BKPT)
> 159 asm("bkpt");
> 160 #endif
>
I am not a fan of injecting code via syscfg, so definitely would prefer
callback approach or just have a function declaraction that application
should define if those callbacks are enabled. - this is quite low level API
so user-friendliness of a callbacks is not something I'd require, but otoh
there may be some scenarios where callbacks could be useful (I don't have
anything specific in mind).
Also refering to what was done in PR, I do not think that having a single
syscfg defined in MCU which controls code in both hal_system and kernel/os
is a good idea. I'd prefer separate options for callback in
hal_system_reset (like MCU_PRE_RESET_CALLBACK) and __assert_func (like
OS_ASSERT_CALLBACK).
These are just some simple suggestions we could follow, obviously there is
> a hard way of doing this thing where we change APIs and register a callback
> and have assert take that callback as an argument but I am not a big fan of
> changing standard APIs.
>
> If others have a suggestion, please let me know. If not I am going to
> assume everybody is fine with option 2.
>
> --
>
> Regards,
> Vipul Rahane
>
Best,
Andrzej
