You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2020/09/30 09:33:05 UTC
[GitHub] [mynewt-core] 5frank opened a new pull request #2382: allow more then one battery
5frank opened a new pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382
Fixes #2380
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#issuecomment-702731673
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] 5frank commented on a change in pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
5frank commented on a change in pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#discussion_r498217537
##########
File path: hw/battery/src/battery.c
##########
@@ -818,7 +823,7 @@ battery_init(struct os_dev *dev, void *arg)
}
os_mutex_release(&battery_manager.bm_lock);
- assert(i < BATTERY_MAX_COUNT);
+ assert(i <= BATTERY_MAX_COUNT);
Review comment:
Agree! Thanks for the feedback! I didn't think that one through
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#issuecomment-702730400
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer commented on a change in pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
kasjer commented on a change in pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#discussion_r498469917
##########
File path: hw/battery/src/battery.c
##########
@@ -25,7 +25,12 @@
#include <battery/battery_prop.h>
#include <battery/battery_drv.h>
+
+#if MYNEWT_VAL(BATTERY_MAX_COUNT)
Review comment:
You are quite right, BATTERY_DRIVERS_MAX is also missing from syscfg.yml.
Here is pull request #2384 that fixes that.
If you still want to contribute your change with syscfg definition please don't hesitate to use force push to your branch so we can limit number of commits with names like `fixes review comments`, and we can merge you commits without squashing them first (that can drop authorship).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] 5frank commented on a change in pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
5frank commented on a change in pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#discussion_r498203416
##########
File path: hw/battery/src/battery.c
##########
@@ -25,7 +25,12 @@
#include <battery/battery_prop.h>
#include <battery/battery_drv.h>
+
+#if MYNEWT_VAL(BATTERY_MAX_COUNT)
Review comment:
Agree that is the right way!
I was following the style you used here:
https://github.com/apache/mynewt-core/blob/f5832e09b0679ce14032850f2d797dc088dbb9fd/hw/battery/include/battery/battery.h#L38-L42
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer merged pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
kasjer merged pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#issuecomment-702730400
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer commented on a change in pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
kasjer commented on a change in pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#discussion_r497800073
##########
File path: hw/battery/src/battery.c
##########
@@ -25,7 +25,12 @@
#include <battery/battery_prop.h>
#include <battery/battery_drv.h>
+
+#if MYNEWT_VAL(BATTERY_MAX_COUNT)
Review comment:
The way to go in that case is to create syscfg definition in **hw/battery/syscfg.yml**.
Without it package uses value that is never defined.
something like that:
```yml
BATTERY_MAX_COUNT:
description: Maximum number of batteries.
value: 1
```
Then it would be also possible to omit
```c
#if MYNEWT_VAL(BATTERY_MAX_COUNT)
```
... and just leave it like this:
```c
#define BATTERY_MAX_COUNT MYNEWT_VAL(BATTERY_MAX_COUNT)
```
##########
File path: hw/battery/src/battery.c
##########
@@ -818,7 +823,7 @@ battery_init(struct os_dev *dev, void *arg)
}
os_mutex_release(&battery_manager.bm_lock);
- assert(i < BATTERY_MAX_COUNT);
+ assert(i <= BATTERY_MAX_COUNT);
Review comment:
This change is not needed. Condition was correct.
On the other hand mutex release should be done later and that could be fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#issuecomment-702581230
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#issuecomment-702731673
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#issuecomment-702581230
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2382: allow more then one battery
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2382:
URL: https://github.com/apache/mynewt-core/pull/2382#issuecomment-702920238
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org