Hi,
Yes, I got your input and will try to factor it in.
- Vipul
On Fri, May 3, 2019 at 12:32 AM Andrzej Kaczmarek <
andrzej.kaczma...@codecoup.pl> wrote:
> 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
>
--
Regards,
Vipul Rahane
