-
Notifications
You must be signed in to change notification settings - Fork 2.1k
gnrc, nimble/ble: notify network layer of BLE connection events #21608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
If a new L2 connection was established, the node should be inserted in the neighbor cache. For 6LN nodes, NIB will never start the usual neighbor discovery process because it can directly resolve the link-layer address from the IPv6 address. Thus we have to manually add such nodes to the NC by building the IPv6 address from the L2 address. If a L2 connection closed, the node is removed again from the NC.
Iterate through all parents whose address match `addr`. In most cases there will only be a single parent, but if `GNRC_RPL_INSTANCES_NUMOF > 1` then one node can be parent in multiple DODAGs.
Handle netapi notification for discovered and unreachable neighbors. For newly discovered neighbors, send a DIS message to the node. For unreachable nodes trigger a timeout for RPL parents that match the address.
`gnrc_rpl` now receives a notification if a new node is reachable (i.e., connected) and sends a DIS. No need anymore to also do it in rpble.
| /** | ||
| * @brief Iterate over all parents in all DODAGs with @p IPv6 address. | ||
| * | ||
| * @param[in] idx Index to start searching from. | ||
| * @param[in] addr IPV6 address of the parent. | ||
| * @param[out] parent Pointer to the parent if one was found. Otherwise NULL. | ||
| * | ||
| * @return Index > 0 to continue next search from, if parent was found. | ||
| * @return -ENONENT if not found | ||
| */ | ||
| int gnrc_rpl_parent_iter_by_addr(ipv6_addr_t *addr, gnrc_rpl_parent_t **parent, int idx); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #21586 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting from there for reviewer's convenience:
This function is a bit weird, but it is needed because, theoretically, if we have more than one RPL instance, the same node can be present as parent in multiple dodags, and thus appear multiple times in the gnrc_rpl_parents list.
We want to mark the parent as unreachable in all dodags.
I'm not knowledgeable enough with RPL to comment on this.
sys/include/net/gnrc/ipv6/nib/nc.h
Outdated
| /** | ||
| * @brief Adds an unmanaged neighbor entry to the NIB if the interface represents a 6LN node | ||
| * and the IPv6 address can be constructed from the L2 address @p l2addr. | ||
| * | ||
| * @param[in] iface The interface to the neighbor. | ||
| * @param[in] l2addr The neighbor's layer 2 address. | ||
| * @param[in] l2addr_len Length of @p l2addr. | ||
| * | ||
| * @return 0 on success. | ||
| * @return -ENOTSUP, if the interface does not represent a 6LN or when | ||
| * gnrc_netif_t::device_type of the iface does not support IID conversion. | ||
| * @return -EINVAL, when @p addr_len is invalid for the | ||
| * gnrc_netif_t::device_type of @p netif. | ||
| * @return -ENOMEM, if no space is left in neighbor cache. | ||
| */ | ||
| int gnrc_ipv6_nib_nc_try_set_6ln(unsigned iface, const uint8_t *l2addr, | ||
| size_t l2addr_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only possible for 6LN nodes because otherwise we can't statically build an IPv6 address from the link-layer address. The IPv6 address is build as a reverse of the _resolve_addr_from_ipv6 that is used when resolving IPv6-> L2address, following the stateless address autoconfiguration section in the RFC.
mguetschow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! Already discussed the design offline, so that looks fine by me. But maybe someone with more knowledge with GNRC and the NETAPI could also comment on the approach (@miri64 maybe?).
Rather high-level comments from my side as I'm not very familiar with GNRC and RPL.
| /** | ||
| * @brief Iterate over all parents in all DODAGs with @p IPv6 address. | ||
| * | ||
| * @param[in] idx Index to start searching from. | ||
| * @param[in] addr IPV6 address of the parent. | ||
| * @param[out] parent Pointer to the parent if one was found. Otherwise NULL. | ||
| * | ||
| * @return Index > 0 to continue next search from, if parent was found. | ||
| * @return -ENONENT if not found | ||
| */ | ||
| int gnrc_rpl_parent_iter_by_addr(ipv6_addr_t *addr, gnrc_rpl_parent_t **parent, int idx); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting from there for reviewer's convenience:
This function is a bit weird, but it is needed because, theoretically, if we have more than one RPL instance, the same node can be present as parent in multiple dodags, and thus appear multiple times in the gnrc_rpl_parents list.
We want to mark the parent as unreachable in all dodags.
I'm not knowledgeable enough with RPL to comment on this.
Weird doxygen parsing error. Co-authored-by: crasbe <[email protected]>
benpicco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds pretty useful as a general mechanism - some first round of comments
| * @param[in] netif The interface to @p l2addr. | ||
| * @param[in] l2addr Layer 2 address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to supply both?
Either you get the l2 address from the netif or you supply the l2 address - then you don't need the netif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The l2 address is not our address on that interface but the remote's address, so we can't get it from the netif.
And the netif parameter itself is needed by l2util_ipv6_iid_from_addr to handle the different types of l2 address properly. Am I missing something?
Use inverse semaphore instead of mutex+condvar+counter.
miri64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursory review after scrolling through most of the code.
sys/net/gnrc/Makefile.dep
Outdated
|
|
||
| ifneq (,$(filter gnrc,$(USEMODULE))) | ||
| USEMODULE += gnrc_netapi | ||
| USEMODULE += gnrc_netapi_notify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also prefer, if it is an optional dependency to gnrc and only in the combination you named mandatory. I would make it a dependency of rplble and gnrc_sixlowpan_6ln combined (not sure these are the correct names of these modules...).
Use blocking `sema_inv_wait`; `sema_inv_try_wait` didn't do anything if we don't want for the result.
Implement single function for copying the event data for all notify event types.
Suggestions from code review. Co-authored-by: Martine Lenders <[email protected]>
Only include netnotify if `nimble_netif` and `gnrc_ipv6` are included.
|
I ran spec07 (multi hop, which includes unmodified RPL) of the release specifications to check if this breaks something. Sadly, it does.Interestingly, also the non-RPL tasks are failing, so I think something in IPv6 is broken. On master it ran fine:(see also the latest run of the weekly release-tests though that is already 4 days old) You can test yourself by cloning https://github.com/RIOT-OS/Release-Specs and running RIOTBASE="<path to RIOT repo>" tox -e test -- -k "spec07" --non-RCin the root directory of that repo. You need an IoT-LAB account for that and be authenticated with that with |
crasbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor style things.
Co-authored-by: crasbe <[email protected]>
Apply suggestions from code review Co-authored-by: crasbe <[email protected]>
miri64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #21608 (comment)
Gate with `IS_USED(MODULE_GNRC_NETAPI_NOTIFY)`.
Add conditional-compilation gates.
Contribution description
This PR solves two issues that have been previously discussed in #21580 and #21586:
Both issues require communication between different netapi layers and are related with each other, so I bundled them in one PR.
Cross-layer Communication with netapi
With the current netapi it is not possible to send arbitrary data between networking threads.
The API only allows to either dispatch packets to multiple threads that are registered in the netreg (
gnrc_netapi_dispatch), or to set/get options to/from a single thread whose PID is known (gnrc_netapi_{get,set}).In my use-case, however, a lower layer (BLE) wants to notify other layers higher up the stack (IPv6, RPL) of an event (e.g., connection established). Ideally, the lower layer shouldn't need to know if/ what higher layers are present, similarly to the case when packets are passed up/ down the stack.
netapi
notifyTo solve the above, I added a new netapi function
gnrc_netapi_notifythat dispatches an "notification"-event to all subscribed threads. Like ingnrc_netapi_dispatchthe target threads are queried from netreg, but withnotifythe sending is done synchronously by blocking for an ACK. The latter is needed because contrary to the packets that get allocated in thepktbuf, the notify event data is stack allocated and thus blocking is needed to ensure the data was read before the function returns. This is not ideal, but I wasn't able to come up with a better solution.Handling BLE connection events
The
notifyevent API is used:In 2. the notification is sent from the IPv6 thread rather than from BLE because address translation from l2addr->ipv6 address is solved on that layer.
Testing procedure
Tested on FIT IoT-Testlab with 9 nrf52dk nodes by using thegnrc_networkingexample +nimble_rpblemodule.The entries are added/ removed from the NIB neighbor cache as expected, and the RPL tree updates when connections between parents and children close.
Edit:
Sorry, misunderstood the section as "how did I test it". Actual test instructions:
USEMODULE += nimble_rpbleto thegnrc_networkingand run on min 2. nodes (I tested with nrf52dk)ifconfig 8 add 2001:db8:1::1/64&&rpl root 1 2001:db8:1::1.ble infoandrplcommands)nib neigh)Issues/PRs references
Replaces #21586.
Part of #21574.
Related to (and might solve) #21580.
Additional Nodes
Happy to split up the PR in separate sub-PRs if that's easier, but as already mentioned it all kinda relates to each other.
See self-review for discussion points.