search

openthread / spinel-spec

Spinel Specification: a general management protocol for enabling a host device to communicate with and manage a Network Control Processor (NCP).
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

core: Reconsider PROP_MAC_SCAN_STATE behavior #4

Open darconeous opened 6 years ago

darconeous commented 6 years ago

Originally, we only had three values for PROP_MAC_SCAN_STATE:

However, the SCAN_STATE_DISCOVER was added in an ill-advised attempt at supporting traditional 802.15.4 beacon scans as well as the new Thread-specific discovery scans. This is confounding to the user. Which one should they use?

I propose we eliminate SCAN_STATE_DISCOVER entirely, and add an entirely new property PROP_MAC_SCAN_ONLY_COMPATIBLE, which would default to true. On a thread device, if this property is true, then doing a beacon scan would actually do a Thread discovery scan. If set to false, it would perform a standard 802.15.4 scan, presumably also returning the results from incompatible networks (to avoid conflicts).

Thoughts?

abtink commented 6 years ago

Discover scan provides additional capabilities and level of control. For example, for discover scan, it's possible to request for "joiner" only, or enable/disable EUI64 filtering and/or PANID filtering. I think it'd be good and useful to keep the different scan types clearly distinct.

In the existing model, there are a bunch of properties that together determine the scan behavior: user needs to set SCAN_MASK, SCAN_PERIOD and then start scan with a write to SCAN_STATE. For discovery scan there are more control which lead to even more properties. There are some limitations with this model:

If we are considering (possibly compatibility breaking) changes to spinel, I would like that that we consider maybe changing this model. My suggestion is to define scan operation as (one or more) spinel command(s) which would then expect a bunch of input arguments (maybe in one or more struct) to control the behavior.

Different scan types can be defined as different commands which makes them distinct (or they can be the same command with a type argument - I prefer former). We can keep the MAC_SCAN_STATE as a read/async-event only property (not writeable anymore).

Thoughts?

darconeous commented 6 years ago

My big concern is that we've taken the "default" way to discover nearby networks and replaced it with a thread-specific mechanism.

Discover scan provides additional capabilities and level of control. For example, for discover scan, it's possible to request for "joiner" only, or enable/disable EUI64 filtering and/or PANID filtering. I think it'd be good and useful to keep the different scan types clearly distinct.

It is fundamentally still a scan for nearby networks. We are also currently not exposing all of those additional capabilities, so I'm not sure how that helps the case for keeping them separate.

In the existing model, there are a bunch of properties that together determine the scan behavior: user needs to set SCAN_MASK, SCAN_PERIOD and then start scan with a write to SCAN_STATE. For discovery scan there are more control which lead to even more properties.

Separate properties for different attributes is a fundamental design decision for Spinel. Doing this helps keep things portable and flexible for the future.

There are some limitations with this model:

  • On the NCP implementation, we need to remember/cache all these values to use for the next scan command.

Aren't we already do this with everything?

  • If the host/user forgets to set a property, then the previously cached value would be used.

The user isn't setting these properties, software is. If a property was set, it means the software was aware of it. If software is setting it (changing the value to something other than the reset value), it can also set it back. Software doesn't forget to do things, humans do.

That being said, we could certainly take a step back and try to re-think things.

If we are considering (possibly compatibility breaking) changes to spinel, I would like that that we consider maybe changing this model. My suggestion is to define scan operation as (one or more) spinel command(s) which would then expect a bunch of input arguments (maybe in one or more struct) to control the behavior.

  • This will simplify the NCP implementation. We get all parameters from the same spinel command frame and start the scan. No need to save/cache the values.
  • It associates all the parameters with a specific scan command.
  • It's more extendable and easier to ensure backward compatibility: A new control parameter can be added at the end of structs and if not provided a reasonable default value can be used.

Different scan types can be defined as different commands which makes them distinct (or they can be the same command with a type argument - I prefer former). We can keep the MAC_SCAN_STATE as a read/async-event only property (not writeable anymore).

You keep implying that a beacon scan and a discovery scan are somehow fundamentally different, but are they not both as mechanisms to find nearby networks? That one sends out 802.15.4 beacon requests and the other sends out a Thread-specific MLE request seems like an implementation detail that the user of this interface shouldn't have to worry about.

My original point was that we should be doing discovery scans by default. But doing scans for all networks that can return beacons is useful during forming (although it is a capability we don't expose) and so I proposed adding an (optional) property for that.

Knowing nothing about the Thread specialization for Spinel, it should be possible for me to successfully perform a scan for nearby networks. What that scan looks like under the hood isn't a huge concern, it just aught to get the job done in a reasonable way.

This whole conversation seems familiar. How are we currently specifying those fields you are referring to?

abtink commented 6 years ago

We actually expose/support all discovery scan capabilities through spinel and they are also supported in wpantund and wpanctl. There are Spinel properties related to discovery scan operation that are set before a discovery scan command. For example, please see:

These are all set for a discovery scan from wpantund SpinelNCPTaskScan.cpp

On the spinel property model for different attributes, I do like it and think it works well in a lot of cases and for controlling network behavior. But for scan operation I sort of feel it may not fit what we would ideally want. Most of other properties/attributes mirror some state/info stored the OpenThread core, so typically we just set/get it directly through an OT API. For scan we need to store the parameters in NcpBase and then invoke the OT scan API with all parameters passed in as API function arguments. The current model works fine but exposing scan as an spinel command with all parameters coming together feels more flexible to me (I know this is bigger change)...

I am fine with discovery scan being the default type of scan, but may driver/wpantund layer can make that selection.

My key point is that at spinel/NCP level, the host/driver is expected to provide/set more attributes for discovery scan compared to a 15.4 beacon scan (e.g., discover MLE scan requires the OT thread interface to be up so if we are offline, SPINEL_PROP_NET_IF_UP should be set true before we can start the scan and then set back to false at end of scan) . The reason I want them to be distinct is to have such differences super clear to avoid confusion/mistake (for a new driver developer)... Maybe we just need to better documentation of this....