xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r934120488
##########
drivers/power/battery_charger.c:
##########
@@ -467,6 +467,16 @@ int battery_charger_changed(FAR struct
battery_charger_dev_s *dev,
FAR struct battery_charger_priv_s *priv;
int ret;
+ /* Event happen too early? */
+
+ if (list_is_clear(&dev->flist))
+ {
+ /* Yes, record it and return directly */
+
+ dev->mask |= mask;
+ return 0;
+ }
Review Comment:
> > > Sorry, maybe I'm missing some use case and failed to locate a call of
`battery_charger_changed` in repository. Maybe you can provide some more
explanation on how such situation could be met?
> >
> >
> > battery_charger_changed is called in the interrupt handler to notify
some interesting event happen.
>
> That is a bit odd design since taking a semaphore from a an interrupt
handler is not the best idea.
>
I simplify the real process: interrupt handler->work
queue->battery_charger_changed
> > > I mean how `battery_charger_changed` could be called before
`battery_charger_register`?
> >
> >
> > The key issue is that battery driver is designed to work immediately
after xxx_initialize since there is no open/activate/enable callback in
battery_charger_operations_s:
https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/power/battery_charger.h#L92-L133
so the driver has to enable the hardware here.
> > The initialization sequence as below:
> >
> > 1. Board file call xxx_initialize to return battery_charger_dev_s like
bq2429x_initialize
> >
https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/power/battery_charger.h#L276-L280
> > 2. Board file call battery_charger_register to register the value
returned from step 1
> > 3. battery_charger_register initialize dev->batsem internally
> >
> > The hardware is enabled in bq2429x_initialize, so it's possible to
receive the interrupt before battery_charger_register finish.
>
> That is strange because I see that `bq2429x_initialize` finishes with
>
> ```
> /* Disable all interrupts. */
>
> ret = bq2429x_en_stat(priv, false);
> ```
>
The notification is added recently. The old driver require the client side
check the new battery state continuously, which isn't acceptable with the
battery powered device, that's why @anjiahao1 create
https://github.com/apache/incubator-nuttx/pull/4655 to support the poll API,
but the old driver need modify the code to gain the new capability.
> So how interrupt can occur before battery_charger_register finishes? I
think that is only possible if your board code does:
>
> ```
> bq2429x_initialize();
> // register and enable (or just enable) interrupt handler that calls
battery_charger_changed and enable interrupts
> battery_charger_register();
> ```
>
> If that is true,
Yes, this is what we do in our driver.
> then it should be easily to modify code to enable interrupt after
`battery_charger_register` is called. Or I'm again missing some use case? I
mean maybe you are talking about your internal driver, but taking `bq2429x` as
an example.
>
It means the driver need expose the internal function to the board files,
which isn't good from the architecture. The board file should just need call
xxx_initialize and xxx_register, no more other special api.
> > > To me it sounds like there is a bug in upper level SW that tries to
access some pre-allocated resource before it has been initialised. If resource
is dynamically allocated, then we do not have guarantee that
`list_is_clear(&dev->flist)` will be evaluated to `true`. If that is statically
allocated,
> >
> >
> > The implementation call either kmm_zalloc or memset, so the list is
always zero.
> > > then I do not see any reason why `SEM_INITIALIZER(1)` can't be used to
init `dev->batsem`.
> >
> >
> > This will cause the batsem initialize twice(here and
battery_charger_register).
>
> Yes, now I see where and how `struct battery_charger_dev_s` is allocated.
>
> In general after studying code more carefully here is my feedback:
>
> 1. `battery_charger_changed` has no interface description neither is
header nor in source fine, so it is very hard to understand what is the context
to involve its processing.
Yes, it's a new interface, old driver need update to take the new
functionality.
> 2. I do not see any reason why `battery_charger_changed` should be called
before `battery_charger_register`. Just including `struct
battery_charger_dev_s` into a struct for inheritance does not mean that `struct
battery_charger_dev_s` is initialized.
As I mention before, there is no interface to enable/disable the driver, the
driver writer has to enable the hardware(include interrupt) in xxx_initialize.
Since the generation of interrupt is out of software control, there is chance
that the interrupt happen before battery_charger_register get chance to
execute. Actually, our driver work very well for more than half year, but
broken recently just because we update the firmware inside the battery monitor
IC which report the event immediately.
> 3. Seems like
[0d95d6fa456](https://github.com/apache/incubator-nuttx/commit/0d95d6fa456) and
[645ff50609a](https://github.com/apache/incubator-nuttx/commit/645ff50609a)
missed to modify `AXP202` case and now it is broken. The `AXP202` did not use
`struct battery_charger_dev_s`, but used `FAR const struct
battery_charger_operations_s *ops;` + `sem_t batsem;` directly in `struct
axp202_dev_s`, so any modification done in `struct battery_charger_dev_s` were
populated into `struct axp202_dev_s`. That should be fixed.
Yes, I will patch this driver.
> 4. I see that `dev->mask` is set, but I do not see where it get cleared.
Maybe `bat_charger_open` should be modified to set `dev->mask = 0;` after
`priv->mask = dev->mask;`? It would be good to get this point clarified.
It's clear in read callback:
https://github.com/apache/incubator-nuttx/pull/6743/files#diff-9c5e91482c4b38cddc223ab7acef3639e13889d6660b8409caec591e74df2aa9R253-R254
> 5.
[0d95d6fa456](https://github.com/apache/incubator-nuttx/commit/0d95d6fa456)
added poll support unconditionally, but maybe it should be configured? I'm fine
for now since nobody complains, so in case if there will be a memory related
concern the config option could be added.
But most driver support poll unconditionally, only the number of poll waiter
can be configured.
> 6.
[645ff50609a](https://github.com/apache/incubator-nuttx/commit/645ff50609a) is
missing some description for `mask`. I suspect that it can contain values from
`BATTERY_XXXX_CHANGED`, but it is only my guessing. It would be good to have
some description in place.
>
Yes, your guess is right.
> It's still a bit hard to understand the full context of an issue that you
are trying to handle, but I suspect that it is "chicken-egg" problem when you
are initializing charging device, register driver and do not want to miss any
events in between. Anyway I think that this problem can be fixed by
initializing a charging device with interrupts disabled, then registering
battery charger driver and implementing enabling of interrupts as one of
`operate` options (extend `enum batio_operate_e` with interrupt enable support).
Yes, this is an approach we can take, but it's an breaking change since all
battery driver doesn't work suddenly without change the client code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
