[GitHub] saulpw commented on a change in pull request #1613: Fix disconnect during OTA by spacing out erases
saulpw commented on a change in pull request #1613: Fix disconnect during OTA
by spacing out erases
URL: https://github.com/apache/mynewt-core/pull/1613#discussion_r250837466
##
File path: mgmt/imgmgr/src/imgmgr.c
##
@@ -739,19 +784,32 @@ imgr_upload(struct mgmt_cbuf *cb)
}
#endif
+#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
+/* setup for lazy sector by sector erase */
+imgr_state.sector_id = -1;
+imgr_state.sector_end = 0;
+#else
+/* erase the entire req.size all at once */
if (action.erase) {
rc = flash_area_erase(fa, 0, req.size);
if (rc != 0) {
rc = MGMT_ERR_EUNKNOWN;
errstr = imgmgr_err_str_flash_erase_failed;
}
}
+#endif
}
/* Write the image data to flash. */
if (rc == 0 && req.data_len != 0) {
-rc = flash_area_write(fa, req.off, req.img_data, action.write_bytes);
-if (rc != 0) {
+#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
+/* erase as we cross sector boundaries */
+if (imgr_erase_if_needed(fa, req.off, action.write_bytes) != 0) {
+rc = MGMT_ERR_EUNKNOWN;
+errstr = imgmgr_err_str_flash_erase_failed;
+} else
Review comment:
Ironically we had a `goto` in the first place and I argued that we should
match the surrounding code style and remove it. Looks like @jerobi was right
in the first place :) Fixed now.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] andrzej-kaczmarek commented on a change in pull request #1613: Fix disconnect during OTA by spacing out erases
andrzej-kaczmarek commented on a change in pull request #1613: Fix disconnect
during OTA by spacing out erases
URL: https://github.com/apache/mynewt-core/pull/1613#discussion_r250833190
##
File path: mgmt/imgmgr/src/imgmgr.c
##
@@ -739,19 +784,32 @@ imgr_upload(struct mgmt_cbuf *cb)
}
#endif
+#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
+/* setup for lazy sector by sector erase */
+imgr_state.sector_id = -1;
+imgr_state.sector_end = 0;
+#else
+/* erase the entire req.size all at once */
if (action.erase) {
rc = flash_area_erase(fa, 0, req.size);
if (rc != 0) {
rc = MGMT_ERR_EUNKNOWN;
errstr = imgmgr_err_str_flash_erase_failed;
}
}
+#endif
}
/* Write the image data to flash. */
if (rc == 0 && req.data_len != 0) {
-rc = flash_area_write(fa, req.off, req.img_data, action.write_bytes);
-if (rc != 0) {
+#if MYNEWT_VAL(IMGMGR_LAZY_ERASE)
+/* erase as we cross sector boundaries */
+if (imgr_erase_if_needed(fa, req.off, action.write_bytes) != 0) {
+rc = MGMT_ERR_EUNKNOWN;
+errstr = imgmgr_err_str_flash_erase_failed;
+} else
Review comment:
such `if`s split by `#ifdef` are quite confusing so I'd prefer to avoid this
by either using `goto` or perhaps just moving this into its own `if (rc == 0 &&
req.data_len != 0)` and `#ifdef` as a whole
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] andrzej-kaczmarek commented on a change in pull request #1613: Fix disconnect during OTA by spacing out erases
andrzej-kaczmarek commented on a change in pull request #1613: Fix disconnect during OTA by spacing out erases URL: https://github.com/apache/mynewt-core/pull/1613#discussion_r250832620 ## File path: mgmt/imgmgr/syscfg.yml ## @@ -30,6 +30,11 @@ syscfg.defs: The maximum amount of image or core data that can fit in a single NMP message value: 512 +IMGMGR_LAZY_ERASE: +description: > +During a firmware upgrade, erase flash a sector at a time +prior to writing to it, rather than all at once at start +value: 1 Review comment: since this changes current behavior it should be disabled by default This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] andrzej-kaczmarek commented on a change in pull request #1613: Fix disconnect during OTA by spacing out erases
andrzej-kaczmarek commented on a change in pull request #1613: Fix disconnect
during OTA by spacing out erases
URL: https://github.com/apache/mynewt-core/pull/1613#discussion_r250832365
##
File path: mgmt/imgmgr/src/imgmgr.c
##
@@ -564,19 +606,22 @@ imgr_upload_inspect(const struct imgr_upload_req *req,
return MGMT_ERR_ENOMEM;
}
+#if MYNEWT_VAL(IMGMGR_LAZY_ERASE) == 0
rc = flash_area_open(action->area_id, );
if (rc) {
*errstr = imgmgr_err_str_flash_open_failed;
return MGMT_ERR_EUNKNOWN;
}
+bool empty;
Review comment:
variables should be declared at beginning of function. you can either
`#ifdef` it there or alternatively use `(void)empty;` so compiler won't
complain - I'm fine with either way of doing this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ccollins476ad merged pull request #261: newt: Remove "imposter packages" from build graph
ccollins476ad merged pull request #261: newt: Remove "imposter packages" from build graph URL: https://github.com/apache/mynewt-newt/pull/261 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[mynewt-newt] branch master updated: newt: Remove "imposter packages" from build graph
This is an automated email from the ASF dual-hosted git repository.
ccollins pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mynewt-newt.git
The following commit(s) were added to refs/heads/master by this push:
new 9bc6d19 newt: Remove "imposter packages" from build graph
9bc6d19 is described below
commit 9bc6d1983976cb362325d19735493f194bbdc752
Author: Christopher Collins
AuthorDate: Wed Jan 23 18:52:43 2019 -0800
newt: Remove "imposter packages" from build graph
A package is an "imposter package" if it is in the dependency graph by
virtue of its own syscfg defines and overrides. For example, say we
have a package `foo`:
pkg.name: foo
syscfg.defs:
FOO_SETTING:
value: 1
Then we have a BSP package:
pkg.name: my_bsp
pkg.deps.FOO_SETTING:
- foo
If this is the only dependency on `foo`, then `foo` is an imposter. It
should be removed from the graph, and its syscfg defines and overrides
should be deleted.
Because the syscfg state changes as newt discovers new dependencies, it
is possible for imposters to end up in the graph.
It is important to remove these packages from the graph for three
reasons:
1. Bogus build failures occur when an imposter package's dependencies
cannot be fully satisfied.
2. If the build succeeds, imposter packages may make silent changes to
the system via syscfg defines and overrides.
3. Increase build time with no benefit.
This commit prunes imposter packages using the following procedure:
A. For each package in the graph, rpkg:
1. Construct a temporary copy of the syscfg. Remove rpkg
entirely from this copy (all its defines and overrides).
2. Attempt to trace rpkg back to a seed package via reverse
dependencies using the modified syscfg.
If an rpkg cannot be traced back to a seed package using the modified
settings (i.e., all links to seed packages are invalid given the
modified config), then it is an imposter and it must be removed.
As will the other pruning procedures, the syscfg and dependency graph
must be recalculated whenever packages are removed. This is so because
each removal potentially changes the syscfg, and consequently may add or
remove dependencies.
---
newt/resolve/resolve.go | 247 +++-
newt/syscfg/syscfg.go | 10 +-
2 files changed, 250 insertions(+), 7 deletions(-)
diff --git a/newt/resolve/resolve.go b/newt/resolve/resolve.go
index 45944c6..5b7f5da 100644
--- a/newt/resolve/resolve.go
+++ b/newt/resolve/resolve.go
@@ -28,6 +28,7 @@ import (
"mynewt.apache.org/newt/newt/flashmap"
"mynewt.apache.org/newt/newt/logcfg"
+ "mynewt.apache.org/newt/newt/newtutil"
"mynewt.apache.org/newt/newt/parse"
"mynewt.apache.org/newt/newt/pkg"
"mynewt.apache.org/newt/newt/project"
@@ -546,7 +547,8 @@ func (r *Resolver) reloadCfg() (bool, error) {
cfg.ResolveValueRefs()
- // Determine if any settings have changed.
+ // Determine if any new settings have been added or if any existing
+ // settings have changed.
for k, v := range cfg.Settings {
oldval, ok := r.cfg.Settings[k]
if !ok || len(oldval.History) != len(v.History) {
@@ -555,9 +557,234 @@ func (r *Resolver) reloadCfg() (bool, error) {
}
}
+ // Determine if any existing settings have been removed.
+ for k, _ := range r.cfg.Settings {
+ if _, ok := cfg.Settings[k]; !ok {
+ r.cfg = cfg
+ return true, nil
+ }
+ }
+
return false, nil
}
+// traceToSeed determines if the specified package can be reached from a seed
+// package via a traversal of the dependency graph. The supplied settings are
+// used to determine the validity of dependencies in the graph.
+func (rpkg *ResolvePackage) traceToSeed(
+ settings map[string]string) (bool, error) {
+
+ seen := map[*ResolvePackage]struct{}{}
+
+ // A nested function is used here so that the `seen` map can be used
across
+ // multiple invocations.
+ var iter func(cur *ResolvePackage) (bool, error)
+ iter = func(cur *ResolvePackage) (bool, error) {
+ // Don't process the same package twice.
+ if _, ok := seen[cur]; ok {
+ return false, nil
+ }
+ seen[cur] = struct{}{}
+
+ // A type greater than "library" is a seed package.
+ if cur.Lpkg.Type() > pkg.PACKAGE_TYPE_LIB {
+ return true, nil
+ }
+
+ // Repeat the trace recursively for each depending package.
+
[GitHub] saulpw opened a new pull request #1613: Fix disconnect during OTA by spacing out erases (#2)
saulpw opened a new pull request #1613: Fix disconnect during OTA by spacing out erases (#2) URL: https://github.com/apache/mynewt-core/pull/1613 * Erasing the entire flash image size at once can take significant time, causing a bluetooth disconnect or significant battery sag. Instead, we erase immediately prior to crossing a sector while writing the image. * We could check for empty to increase efficiency. However, for simplicity and consistency we will always erase lazily. * wrap with `MYNEWT_VAL(IMGMGR_LAZY_ERASE)`. Set IMGMGR_LAZY_ERASE to 0 to disable, 1 to enable. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk opened a new pull request #307: btshell: Fix controller reset on test-tx command
rymanluk opened a new pull request #307: btshell: Fix controller reset on test-tx command URL: https://github.com/apache/mynewt-nimble/pull/307 This patch fixes test-tx command which was ending with controller reset because data were send omiting bhc_outstanding_pkts needed for correct handling number of completed packets. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ccollins476ad edited a comment on issue #260: [RFC] `$import` directive
ccollins476ad edited a comment on issue #260: [RFC] `$import` directive
URL: https://github.com/apache/mynewt-newt/pull/260#issuecomment-457392854
Thanks @benmccrea, those are good questions. It turns out my testing of
this feature left much to be desired; I discovered a bug while coming up with a
response! I have pushed the fix.
I am using different settings here than the specific ones you asked about
(`OS_DEBUG_MODE`->`HAL_FLASH_VERIFY_ERASES`,
`OICMGR_TRANS_SECURITY`->`HAL_FLASH_VERIFY_BUF_SZ`). This allows a more basic
target with a simpler syscfg.
## Setup 1
There are three files:
`targets/my_blinky_sim/syscfg.yml`:
```
$import:
- 'misc/importa.yml'
- 'misc/importb.yml'
syscfg.vals:
HAL_FLASH_VERIFY_ERASES: 1
```
`misc/importa.yml`:
```
syscfg.vals:
HAL_FLASH_VERIFY_BUF_SZ: 32
```
`misc/importb.yml`:
```
syscfg.vals:
HAL_FLASH_VERIFY_BUF_SZ: 16
```
Newt generated the following HAL syscfg for this target:
```
[@apache-mynewt-core/hw/hal]
HAL_FLASH_VERIFY_BUF_SZ: 16 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_ERASES: 1 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_WRITES: 0
```
If we reverse the order of the imports (i.e., `importb` before `importa`),
then the syscfg changes:
```
[@apache-mynewt-core/hw/hal]
HAL_FLASH_VERIFY_BUF_SZ: 32 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_ERASES: 1 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_WRITES: 0
```
So, import order matters, which I think is the correct behavior. This is
something that should be documented. Imports are processed in the specified
order, then the importing file is processed.
## Setup 2
Two files:
`targets/my_blinky_sim/syscfg.yml`:
```
$import:
- 'misc/importa.yml'
syscfg.vals:
HAL_FLASH_VERIFY_ERASES: 1
```
`misc/importa.yml`:
```
syscfg.vals:
HAL_FLASH_VERIFY_ERASES: 0
```
Newt generated the following HAL syscfg for this target:
```
[@apache-mynewt-core/hw/hal]
HAL_FLASH_VERIFY_BUF_SZ: 16
HAL_FLASH_VERIFY_ERASES: 1 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_WRITES: 0
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ccollins476ad commented on issue #260: [RFC] `$import` directive
ccollins476ad commented on issue #260: [RFC] `$import` directive
URL: https://github.com/apache/mynewt-newt/pull/260#issuecomment-457392854
Thanks @benmccrea, those are good questions. It turns out my testing of
this feature left much to be desired; I discovered a bug while coming up with a
response! I have pushed the fix.
I am using different settings here than the specific ones you asked about
(`OS_DEBUG_MODE`->`HAL_FLASH_VERIFY_ERASES`,
`OICMGR_TRANS_SECURITY`->`HAL_FLASH_VERIFY_BUF_SZ`). This allows a more basic
target with a simpler syscfg. There are three files:
`targets/my_blinky_sim/syscfg.yml`:
```
$import:
- 'misc/importa.yml'
- 'misc/importb.yml'
syscfg.vals:
HAL_FLASH_VERIFY_ERASES: 1
```
`misc/importa.yml`:
```
syscfg.vals:
HAL_FLASH_VERIFY_BUF_SZ: 32
```
`misc/importb.yml`:
```
syscfg.vals:
HAL_FLASH_VERIFY_BUF_SZ: 16
```
Newt generated the following HAL syscfg for this target:
```
[@apache-mynewt-core/hw/hal]
HAL_FLASH_VERIFY_BUF_SZ: 16 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_ERASES: 1 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_WRITES: 0
```
If we reverse the order of the imports (i.e., `importb` before `importa`),
then the syscfg changes:
```
[@apache-mynewt-core/hw/hal]
HAL_FLASH_VERIFY_BUF_SZ: 32 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_ERASES: 1 (overridden by targets/my_blinky_sim)
HAL_FLASH_VERIFY_WRITES: 0
```
So, import order matters, which I think is the correct behavior. This is
something that should be documented. Imports are processed in the specified
order, then the importing file is processed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] andrzej-kaczmarek merged pull request #306: nimble/host: Restore key size with existing LTK
andrzej-kaczmarek merged pull request #306: nimble/host: Restore key size with existing LTK URL: https://github.com/apache/mynewt-nimble/pull/306 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[mynewt-nimble] branch master updated: nimble/host: Restore key size for LTK
This is an automated email from the ASF dual-hosted git repository.
andk pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mynewt-nimble.git
The following commit(s) were added to refs/heads/master by this push:
new e3b9d73 nimble/host: Restore key size for LTK
e3b9d73 is described below
commit e3b9d73db0c9d2c8efd47d0d5eaea9c6673752e7
Author: Andrzej Kaczmarek
AuthorDate: Wed Jan 23 17:58:13 2019 -0800
nimble/host: Restore key size for LTK
When LTK is restored successfully we also need to update key size for
proc with proper value as otherwise we default to 0 which means that
e.g. any characteristic which requires proper key size for read/write
cannot be accessed.
---
nimble/host/src/ble_sm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/nimble/host/src/ble_sm.c b/nimble/host/src/ble_sm.c
index d0d464d..1744832 100644
--- a/nimble/host/src/ble_sm.c
+++ b/nimble/host/src/ble_sm.c
@@ -1306,6 +1306,7 @@ ble_sm_ltk_restore_exec(struct ble_sm_proc *proc, struct
ble_sm_result *res,
proc->conn_handle, value_sec->ltk);
if (res->app_status == 0) {
+proc->key_size = value_sec->key_size;
if (value_sec->authenticated) {
proc->flags |= BLE_SM_PROC_F_AUTHENTICATED;
}
[GitHub] ccollins476ad commented on issue #6: BLE reassembly support
ccollins476ad commented on issue #6: BLE reassembly support URL: https://github.com/apache/mynewt-mcumgr/issues/6#issuecomment-457310275 @bgiori Re: Write Long Characteristic Values procedure- My "BLE" is a bit rusty, so please correct me if I am wrong. During a long write procedure, each fragment must be acknowledged by the server (Prepare Write Response). The protocol would still effectively be stop-and-wait, so I am not sure this would help much with performance. I think we would want to do this at the application layer. Then we can still use unacknowledged writes. I can think of two ways we may want to achieve this (my apologies if I am rehashing things that have already been discussed): **1. Generic fragmentation mechanism.** This is probably what you'd expect: the server buffers each fragment until the entire packet is received, then reassembles and processes. *Good:* Works for all SMP command types. *Bad:* Requires a lot of buffer space on the server. **2. DFU-specific mechanism.** The image upload requests are not fragmented. Instead, the requests are selectively acknowledged. The server writes image chunks to flash as it receives them, but only acknowledges every X requests (or every X bytes). Both sides understand the acknowledgement policy, so the client can rapidly send X requests, then listen for a response. *Good:* Does not require extra buffer space on the server. *Bad:* a) Only helps with DFU; b) probably more complicated to implement. Thoughts? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ccollins476ad commented on issue #17: Handle unexpected poweroff
ccollins476ad commented on issue #17: Handle unexpected poweroff URL: https://github.com/apache/mynewt-nffs/issues/17#issuecomment-457293165 For the record, this is meant to work, and there are unit tests which exercise this functionality. I have not analyzed at the data in the zephyr bug report, but if this is broken in NFFS, then it requires a bug fix (as opposed to a new feature). This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250656047 ## File path: nimble/host/include/host/ble_gap.h ## @@ -969,23 +969,23 @@ int ble_gap_ext_adv_remove(uint8_t instance); #if MYNEWT_VAL(BLE_PERIODIC_ADV) /* Periodic Advertising */ -int ble_gap_per_adv_configure(uint8_t instance, const struct ble_gap_per_adv_params *params); -int ble_gap_per_adv_start(uint8_t instance); -int ble_gap_per_adv_stop(uint8_t instance); -int ble_gap_per_adv_set_data(uint8_t instance, struct os_mbuf *data); -int ble_gap_per_adv_create_sync (uint8_t filter_policy, +int ble_gap_periodic_adv_configure(uint8_t instance, const struct ble_gap_periodic_adv_params *params); +int ble_gap_periodic_adv_start(uint8_t instance); +int ble_gap_periodic_adv_stop(uint8_t instance); +int ble_gap_periodic_adv_set_data(uint8_t instance, struct os_mbuf *data); +int ble_gap_periodic_adv_create_sync (uint8_t filter_policy, Review comment: indentation issue. Tabs used This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250689134 ## File path: nimble/host/src/ble_gap.c ## @@ -1355,7 +1377,7 @@ ble_gap_rx_per_adv_sync_estab(struct hci_le_subev_per_adv_sync_estab *evt) event.per_adv_sync_estab.per_adv_ival = evt->per_adv_ival; event.per_adv_sync_estab.adv_clk_accuracy = evt->adv_clk_accuracy; - ble_gap_master.pending_create_sync--; + ble_gap_master.pending_create_sync = 0; Review comment: `pending_create_sync` is `uint32_t`. Can be reduced. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic
Advertising Feature
URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250689904
##
File path: nimble/host/src/ble_gap.c
##
@@ -1388,23 +1410,33 @@ ble_gap_rx_per_adv_rpt(struct hci_le_subev_per_adv_rpt
*evt) {
}
void
-ble_gap_rx_per_adv_sync_lost(struct hci_le_subev_per_adv_sync_lost *evt) {
-
+ble_gap_rx_periodic_adv_sync_lost(
+ struct hci_le_subev_periodic_adv_sync_lost *evt) {
struct ble_gap_event event;
struct ble_gap_master_state state;
+struct ble_hs_periodic_disc *pdisc;
memset(, 0, sizeof event);
- event.type = BLE_GAP_EVENT_PER_ADV_SYNC_LOST;
+ event.type = BLE_GAP_EVENT_PERIODIC_ADV_SYNC_LOST;
event.per_adv_sync_lost.sync_handle = evt->sync_handle;
+ ble_hs_lock();
+ /* The handle must be in the list */
+ pdisc = ble_hs_periodic_disc_find_assert(evt->sync_handle);
+
+ /* Remove the handle from the list */
+ ble_hs_periodic_disc_remove(pdisc);
Review comment:
maybe `ble_hs_periodic_sync_remove` and the same with other functions related
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic
Advertising Feature
URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250681768
##
File path: nimble/host/src/ble_gap.c
##
@@ -1336,14 +1336,36 @@ ble_gap_rx_adv_set_terminated(struct
hci_le_adv_set_terminated *evt)
/* Periodic adv events */
#if MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_PERIODIC_ADV)
void
-ble_gap_rx_per_adv_sync_estab(struct hci_le_subev_per_adv_sync_estab *evt)
+ble_gap_rx_peroidic_adv_sync_estab(
+ struct hci_le_subev_periodic_adv_sync_estab *evt)
{
struct ble_gap_event event;
struct ble_gap_master_state state;
+ struct ble_hs_periodic_disc * pdisc;
memset(, 0, sizeof event);
- event.type = BLE_GAP_EVENT_PER_ADV_SYNC_ESTAB;
+ /* There must be memory for pdisc as the check was done when creating
+* sync
+*/
+ pdisc = ble_hs_periodic_disc_alloc(evt->sync_handle);
Review comment:
sync_handle seems to be written to pdisc twice. I would remove sync_handle
from `ble_hs_periodic_disc_alloc`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic
Advertising Feature
URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250682665
##
File path: nimble/host/src/ble_gap.c
##
@@ -1336,14 +1336,36 @@ ble_gap_rx_adv_set_terminated(struct
hci_le_adv_set_terminated *evt)
/* Periodic adv events */
#if MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_PERIODIC_ADV)
void
-ble_gap_rx_per_adv_sync_estab(struct hci_le_subev_per_adv_sync_estab *evt)
+ble_gap_rx_peroidic_adv_sync_estab(
+ struct hci_le_subev_periodic_adv_sync_estab *evt)
{
struct ble_gap_event event;
struct ble_gap_master_state state;
+ struct ble_hs_periodic_disc * pdisc;
Review comment:
`struct ble_hs_periodic_disc` is to describe `sync` so what about `struct
ble_hs_periodic_sync` ? and then variable maybe `psync`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250625641 ## File path: nimble/include/nimble/hci_common.h ## @@ -984,12 +1001,23 @@ struct hci_ext_adv_params uint8_t scan_req_notif; }; + Review comment: please remove redundant empty line This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250688315 ## File path: nimble/host/include/host/ble_gap.h ## @@ -969,23 +969,23 @@ int ble_gap_ext_adv_remove(uint8_t instance); #if MYNEWT_VAL(BLE_PERIODIC_ADV) /* Periodic Advertising */ -int ble_gap_per_adv_configure(uint8_t instance, const struct ble_gap_per_adv_params *params); -int ble_gap_per_adv_start(uint8_t instance); -int ble_gap_per_adv_stop(uint8_t instance); -int ble_gap_per_adv_set_data(uint8_t instance, struct os_mbuf *data); -int ble_gap_per_adv_create_sync (uint8_t filter_policy, +int ble_gap_periodic_adv_configure(uint8_t instance, const struct ble_gap_periodic_adv_params *params); +int ble_gap_periodic_adv_start(uint8_t instance); +int ble_gap_periodic_adv_stop(uint8_t instance); +int ble_gap_periodic_adv_set_data(uint8_t instance, struct os_mbuf *data); +int ble_gap_periodic_adv_create_sync (uint8_t filter_policy, Review comment: Did you consider to add callback to `ble_gap_periodic_adv_create_sync()` for periodic advertising events? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250629567 ## File path: nimble/host/src/ble_hs_hci_priv.h ## @@ -235,6 +235,48 @@ ble_hs_hci_cmd_build_le_ext_adv_params(uint8_t handle, int ble_hs_hci_cmd_build_le_ext_adv_remove(uint8_t handle, uint8_t *cmd, int cmd_len); + +#if MYNEWT_VAL(BLE_PERIODIC_ADV) +int +ble_hs_hci_cmd_build_le_per_adv_params(uint8_t handle, + const struct hci_per_adv_params *params, Review comment: Indentation issue (tab used instead of spaces) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic
Advertising Feature
URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250683320
##
File path: nimble/host/src/ble_gap.c
##
@@ -1336,14 +1336,36 @@ ble_gap_rx_adv_set_terminated(struct
hci_le_adv_set_terminated *evt)
/* Periodic adv events */
#if MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_PERIODIC_ADV)
void
-ble_gap_rx_per_adv_sync_estab(struct hci_le_subev_per_adv_sync_estab *evt)
+ble_gap_rx_peroidic_adv_sync_estab(
+ struct hci_le_subev_periodic_adv_sync_estab *evt)
{
struct ble_gap_event event;
struct ble_gap_master_state state;
+ struct ble_hs_periodic_disc * pdisc;
memset(, 0, sizeof event);
- event.type = BLE_GAP_EVENT_PER_ADV_SYNC_ESTAB;
+ /* There must be memory for pdisc as the check was done when creating
+* sync
+*/
+ pdisc = ble_hs_periodic_disc_alloc(evt->sync_handle);
+ BLE_HS_DBG_ASSERT(pdisc != NULL);
+
+ pdisc->sync_handle = evt->sync_handle;
+ pdisc->adv_sid = evt->sid;
+ pdisc->advertiser_addr_type = evt->adv_addr_type;
+
+ memcpy(pdisc->advertiser_addr.val, evt->adv_addr, 6);
+
+ pdisc->advertiser_phy= evt->adv_phy;
+ pdisc->periodic_adv_itvl = evt->per_adv_ival;
+ pdisc->advertiser_clock_accuracy = evt->adv_clk_accuracy;
+
+ ble_hs_lock();
+ ble_hs_periodic_disc_insert(pdisc);
+ ble_hs_unlock();
+
+ event.type = BLE_GAP_EVENT_PERIODIC_ADV_SYNC_ESTAB;
Review comment:
did you consider having in the event only `sync_handle` and then some other
API to get information? Maybe something more is immediately needed by
applications (e.g. sid) but I I guess not all of them
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic
Advertising Feature
URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250625758
##
File path: nimble/include/nimble/hci_common.h
##
@@ -984,12 +1001,23 @@ struct hci_ext_adv_params
uint8_t scan_req_notif;
};
+
/* LE Extended Advertising Report Event */
struct hci_le_subev_ext_adv_rpt {
uint8_t num_reports;
struct hci_ext_adv_report_param params[0];
} __attribute__((packed));
+
Review comment:
please remove redundant empty line
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic
Advertising Feature
URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250695263
##
File path: nimble/host/src/ble_gap.c
##
@@ -1336,14 +1336,36 @@ ble_gap_rx_adv_set_terminated(struct
hci_le_adv_set_terminated *evt)
/* Periodic adv events */
#if MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_PERIODIC_ADV)
void
-ble_gap_rx_per_adv_sync_estab(struct hci_le_subev_per_adv_sync_estab *evt)
+ble_gap_rx_peroidic_adv_sync_estab(
+ struct hci_le_subev_periodic_adv_sync_estab *evt)
{
struct ble_gap_event event;
struct ble_gap_master_state state;
+ struct ble_hs_periodic_disc * pdisc;
memset(, 0, sizeof event);
- event.type = BLE_GAP_EVENT_PER_ADV_SYNC_ESTAB;
+ /* There must be memory for pdisc as the check was done when creating
+* sync
+*/
+ pdisc = ble_hs_periodic_disc_alloc(evt->sync_handle);
+ BLE_HS_DBG_ASSERT(pdisc != NULL);
+
+ pdisc->sync_handle = evt->sync_handle;
+ pdisc->adv_sid = evt->sid;
+ pdisc->advertiser_addr_type = evt->adv_addr_type;
+
+ memcpy(pdisc->advertiser_addr.val, evt->adv_addr, 6);
+
+ pdisc->advertiser_phy= evt->adv_phy;
+ pdisc->periodic_adv_itvl = evt->per_adv_ival;
+ pdisc->advertiser_clock_accuracy = evt->adv_clk_accuracy;
+
+ ble_hs_lock();
+ ble_hs_periodic_disc_insert(pdisc);
Review comment:
btw - please check `ble_hs_stop` where we disconnect all the connections on
stack close. We need to add there code t terminate all periodic_sync
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250625027 ## File path: porting/nimble/include/syscfg/syscfg.h ## @@ -378,7 +378,7 @@ /*** nimble */ #ifndef MYNEWT_VAL_BLE_EXT_ADV -#define MYNEWT_VAL_BLE_EXT_ADV (0) Review comment: we don't want this change for porting version. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature
rymanluk commented on a change in pull request #283: Adding Periodic Advertising Feature URL: https://github.com/apache/mynewt-nimble/pull/283#discussion_r250625159 ## File path: porting/nimble/include/syscfg/syscfg.h ## @@ -418,6 +418,11 @@ #define MYNEWT_VAL_BLE_ATT_PREFERRED_MTU (256) #endif + +#ifndef MYNEWT_VAL_BLE_PERIODIC_ADV +#define MYNEWT_VAL_BLE_PERIODIC_ADV (1) Review comment: For porting I would got for 0 as a default. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] andrzej-kaczmarek commented on issue #303: nimble/ll: Fix access address computation
andrzej-kaczmarek commented on issue #303: nimble/ll: Fix access address computation URL: https://github.com/apache/mynewt-nimble/pull/303#issuecomment-457234521 agree, although with proposed modification we do quite a lot of unnecessary bit counting. instead we can just find all transitions with a simple xor and then just count bits once. I'll send a patch in a moment. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
