You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/07/30 03:46:09 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #6743: drivers/battery: Handle the early changed event correctly
xiaoxiang781216 opened a new pull request, #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743
## Summary
since it may happen before battery_xxx_register sometime
## Impact
Some corner case
## Testing
Internal test
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#issuecomment-1202667242
> Adding `battery_charger_initialize` that will init semaphore and the list (move init part from `battery_charger_register`) and calling it from `bq2429x_initialize` is a solution that will fix the problem and keep the existing behaviour.
Both approach need change bq2429x_initialize, either is fine, but it look a little bit redundancy to has two initialize functions.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#issuecomment-1202656748
Adding `battery_charger_initialize` that will init semaphore and the list (move init part from `battery_charger_register`) and calling it from `bq2429x_initialize` is a solution that will fix the problem and keep the existing behaviour.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#issuecomment-1202631429
> Actually, I prefer the driver call battery_charger_register inside the initialization function(e.g. bq2429x_initialize), so it can arrange the code that call battery_charger_register before enable the interrupt. But, the change in this PR is a safe guard to make sure that old driver work correctly in the extreme case.
I am okay with that. Thanks.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933981483
##########
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.
> 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.
> 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).
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933777415
##########
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:
The event may happen before sem init here:
https://github.com/apache/incubator-nuttx/pull/6743/files#diff-9c5e91482c4b38cddc223ab7acef3639e13889d6660b8409caec591e74df2aa9R521
This is the root cause to make this change to avoid the thread blocking forever.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r934037164
##########
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 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);
```
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, 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.
> > 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.
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.
3. Seems like https://github.com/apache/incubator-nuttx/commit/0d95d6fa456 and 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.
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.
5. 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.
6. 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.
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).
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#issuecomment-1202675187
@xiaoxiang781216 @hartmannathan so what is the plan? Just go with existing change, or merging this and then introducing a "breaking change" to call `battery_charger_register` from `bq2429x_initialize`?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933965859
##########
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? I mean how `battery_charger_changed` could be called before `battery_charger_register`? 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. I mean 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, then I do not see any reason why `SEM_INITIALIZER(1)` can't be used to init `dev->batsem`.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
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 a 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#issuecomment-1202788887
It's better to do the breaking change in the new PR, thanks.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933965859
##########
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? I mean how `battery_charger_changed` could be called before `battery_charger_register`? I mean 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. I mean 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, then I do not see any reason why `SEM_INITIALIZER(1)` can't be used to init `dev->batsem`.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r934037164
##########
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 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);
```
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, 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.
> > 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.
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.
3. Seems like https://github.com/apache/incubator-nuttx/commit/0d95d6fa456#diff-603fcaec9ab084384b123ac88eb8b239b1f19f4a79e8f72d912d658eba0284b7 and https://github.com/apache/incubator-nuttx/commit/645ff50609a#diff-603fcaec9ab084384b123ac88eb8b239b1f19f4a79e8f72d912d658eba0284b7 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.
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.
5. https://github.com/apache/incubator-nuttx/commit/0d95d6fa456#diff-603fcaec9ab084384b123ac88eb8b239b1f19f4a79e8f72d912d658eba0284b7 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.
6. https://github.com/apache/incubator-nuttx/commit/645ff50609a#diff-603fcaec9ab084384b123ac88eb8b239b1f19f4a79e8f72d912d658eba0284b7 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.
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).
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933965859
##########
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? I mean how `battery_charger_changed` could be called before `battery_charger_register`? 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, then I do not see any reason why `SEM_INITIALIZER(1)` can't be used to init `dev->batsem`.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933800175
##########
include/nuttx/list.h:
##########
@@ -390,6 +390,11 @@ static inline bool list_is_empty(FAR struct list_node *list)
return (list->next == list) ? true : false;
}
+static inline bool list_is_clear(FAR struct list_node *list)
+{
+ return (list->next == NULL) ? true : false;
Review Comment:
Done.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933777486
##########
include/nuttx/list.h:
##########
@@ -390,6 +390,11 @@ static inline bool list_is_empty(FAR struct list_node *list)
return (list->next == list) ? true : false;
}
+static inline bool list_is_clear(FAR struct list_node *list)
+{
+ return (list->next == NULL) ? true : false;
Review Comment:
This just follow the style of list_is_empty, I think the consistent is more important.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r934370363
##########
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
Understood.
> > > > 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 #4655 to support the poll API, but the old driver need modify the code to gain the new capability.
Do you plan to upstream the interrupt version of the driver?
> > 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.
Are you calling `battery_charger_changed` from `bq2429x` driver code? If `bq2429x` driver and battery driver become tightly coupled in case of interrupt mode, then maybe we can modify battery driver code and call xxx_register from inside the xxx_initialize? Otherwise it is counterintuitive (at least to me) that `battery_charger_` APIs could be called before `battery_charger_register` and should gracefully handle such a behaviour.
> > > > 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.
Could you please take over and add the description?
> > 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.
Again, that is partially discussed above. I'm still missing there the interrupt handler lives. If it is at `bq2429x` driver layer, then maybe it would be good to call `xxx_register` from `xxx_initialize`. If the interrupt handler is at board level, then board know the specific and code could be like:
```
struct battery_charger_dev_s *g_changer_dev;
struct battery_charger_dev_s *dev = bq2429x_initialize();
battery_charger_register("/dev/batX", dev);
g_changer_dev = dev;
```
and interrupt chandler would check for `g_changer_dev != NULL;`.
Personally I think that we should call `xxx_register` from `xxx_initialize` in case if `devpath` is not null.
> > 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.
Thank you!
> > 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
That is `priv->mask`, but not `dev->mask`. Are those the same? I think not, because of `priv->mask = dev->mask;` in `bat_charger_open`.
> > 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.
Ok.
> > 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 a 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r935362689
##########
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:
> Let's keep an "clear-list" protection, but I'm still not happy with such design solution :)
>
> > dev->mask accumulate all events from start up and never clear.
> > priv->mask copy from dev->mask in open, accumulate when new event come in and clear in read. This sequence fix:
> >
> > 1. The event may happen before the user space daemon open device node, and then lose these events
> > 2. Clearing mask in read could avoid the busy poll loop.
>
> So with each `open -> close -> open` sequence the events from early start will be propagated to `priv->mask`?
Yes, this is to simplify the userspace code.
> Could you please explain more why this is needed?
If we don't copy the current mask in open, the caller has to:
1. Open "/dev/xxx"
2. Initialize the current stats by a bunch of ioctl
3. Call poll to wait the new stats happen
4. Update the current stats by a bunch of ioctl
You can see step 2 and 4 is duplicated. On the other hand, with the current implementation:
1. Open "/dev/xxx"
2. Call poll to wait the new stats happen
3. Update the current stats by a bunch of ioctl
The initial call of poll return immediately, and the stats get the last value automatically.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#issuecomment-1202596465
Actually, I prefer the driver call battery_charger_register inside the initialization function(e.g. bq2429x_initialize), so it can arrange thee code that call battery_charger_register before enable the interrupt. But, the change in this PR is a safe guard to make sure the old driver work correctly in the extreme case.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933981483
##########
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.
> 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
And 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.
> 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).
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r935403377
##########
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:
> > Let's keep an "clear-list" protection, but I'm still not happy with such design solution :)
> > > dev->mask accumulate all events from start up and never clear.
> > > priv->mask copy from dev->mask in open, accumulate when new event come in and clear in read. This sequence fix:
> > >
> > > 1. The event may happen before the user space daemon open device node, and then lose these events
> > > 2. Clearing mask in read could avoid the busy poll loop.
> >
> >
> > So with each `open -> close -> open` sequence the events from early start will be propagated to `priv->mask`?
>
> Yes, this is to simplify the userspace code.
>
> > Could you please explain more why this is needed?
>
> If we don't copy the current mask in open, the caller has to:
>
> 1. Open "/dev/xxx"
> 2. Initialize the current stats by a bunch of ioctl
> 3. Call poll to wait the new stats happen
> 4. Update the current stats by a bunch of ioctl
>
> You can see step 2 and 4 is duplicated. On the other hand, with the current implementation:
>
> 1. Open "/dev/xxx"
> 2. Call poll to wait the new stats happen
> 3. Update the current stats by a bunch of ioctl
>
> The initial call of poll return immediately, and the stats get the last value automatically.
But if `BATTERY_STATE_CHANGED` happened after driver was registered before first `open` does not mean that `BATTERY_STATE_CHANGED` happened between `close` and second `open`. So each `open` will populate event from start of the system even if those do not happen between `close` to `open`. Anyway I will leave this to you since you have the full infra to test it.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933758869
##########
include/nuttx/list.h:
##########
@@ -390,6 +390,11 @@ static inline bool list_is_empty(FAR struct list_node *list)
return (list->next == list) ? true : false;
}
+static inline bool list_is_clear(FAR struct list_node *list)
+{
+ return (list->next == NULL) ? true : false;
Review Comment:
```suggestion
return (list->next == NULL);
```
##########
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:
How correct is to call this before taking `dev->batsem`?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r933785692
##########
include/nuttx/list.h:
##########
@@ -390,6 +390,11 @@ static inline bool list_is_empty(FAR struct list_node *list)
return (list->next == list) ? true : false;
}
+static inline bool list_is_clear(FAR struct list_node *list)
+{
+ return (list->next == NULL) ? true : false;
Review Comment:
I think list_is_empty should be modified as well to keep the style. The result of `==` is already `true` or `false` so usage of the trinary operator here is redundant.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
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.
> 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r934664548
##########
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:
> > 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 #4655 to support the poll API, but the old driver need modify the code to gain the new capability.
>
> Do you plan to upstream the interrupt version of the driver?
>
It isn't a mainlined battery driver and still in the testing(bq2429x is just an example), so it need time to prepare the patch.
> > 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.
>
> Are you calling `battery_charger_changed` from `bq2429x` driver code?
Not for bq2429x, but an internal battery driver for new hardware
> If `bq2429x` driver and battery driver become tightly coupled in case of interrupt mode, then maybe we can modify battery driver code and call xxx_register from inside the xxx_initialize?
Yes, this is another approach fix that we call xxx_register first in xxx_initialize before enabling the interrupt. But, again it require change the public function prototype.
> Otherwise it is counterintuitive (at least to me) that `battery_charger_` APIs could be called before `battery_charger_register` and should gracefully handle such a behaviour.
>
>
> Could you please take over and add the description?
>
Ok, @anjiahao1 please add the comment for new function and mask.
>
> Again, that is partially discussed above. I'm still missing there the interrupt handler lives. If it is at `bq2429x` driver layer, then maybe it would be good to call `xxx_register` from `xxx_initialize`. If the interrupt handler is at board level, then board know the specific and code could be like:
>
> ```
> struct battery_charger_dev_s *g_changer_dev;
>
> struct battery_charger_dev_s *dev = bq2429x_initialize();
> battery_charger_register("/dev/batX", dev);
> g_changer_dev = dev;
> ```
>
> and interrupt chandler would check for `g_changer_dev != NULL;`. Personally I think that we should call `xxx_register` from `xxx_initialize` in case if `devpath` is not null.
>
The solution isn't good since we can't support the multiple instance now.
> > > 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
>
> That is `priv->mask`, but not `dev->mask`. Are those the same? I think not, because of `priv->mask = dev->mask;` in `bat_charger_open`.
dev->mask accumulate all events from start up and never clear.
priv->mask copy from dev->mask in open, accumulate when new event come in and clear in read. This sequence fix:
1. The event may happen before the user space daemon open device node, and then lose these events
2. Clearing mask in read could avoid the busy poll loop.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r935308055
##########
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:
Let's keep an "clear-list" protection, but I'm still not happy with such design solution :)
> dev->mask accumulate all events from start up and never clear.
priv->mask copy from dev->mask in open, accumulate when new event come in and clear in read. This sequence fix:
>
> 1. The event may happen before the user space daemon open device node, and then lose these events
> 2. Clearing mask in read could avoid the busy poll loop.
So with each `open -> close -> open` sequence the events from early start will be propagated to `priv->mask`? Could you please explain more why this is needed?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6743: drivers/battery: Handle the early changed event correctly
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6743:
URL: https://github.com/apache/incubator-nuttx/pull/6743#discussion_r935530297
##########
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:
It is no an issue: the new user or the restarted server always need update the internal stats with the hardware anyway since it lose the info for the second case.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org