Skip to content
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

Use a wizard for creating end devices #579

Closed
kschiffer opened this issue Apr 28, 2019 · 38 comments
Closed

Use a wizard for creating end devices #579

kschiffer opened this issue Apr 28, 2019 · 38 comments
Assignees
Labels
c/console This is related to the Console in progress We're working on it ui/web This is related to a web interface
Milestone

Comments

@kschiffer
Copy link
Member

Summary:
The add device form (added in #573) is currently not composing fields based on constraints (except for ABP / OTAA selection). For example, it could hide the fields that are only relevant for specific LoRaWAN Versions, or rename fields accordingly (eg. NwkSKey vs. FNwkSIntKey).

Why do we need this?
For better UX, avoid faulty inputs

What is already there? What do you see now?
The add device form, with conditional fields based on OTAA/ABP

What is missing? What do you want to see?
More sophisticated checks and constraints applied into the form, preventing faulty inputs

Environment:
Console in browser

How do you propose to implement this?
Likely hook into field change events and compose fields accordingly

Can you do this yourself and submit a Pull Request?
Yes.

@kschiffer kschiffer added c/console This is related to the Console goldplating This is just polishing labels Apr 28, 2019
@kschiffer kschiffer added this to the Backlog milestone Apr 28, 2019
@kschiffer kschiffer self-assigned this Apr 28, 2019
@johanstokking johanstokking added the ui/web This is related to a web interface label Jul 19, 2019
@bafonins bafonins assigned bafonins and unassigned kschiffer Aug 1, 2019
@bafonins bafonins added the in progress We're working on it label Aug 1, 2019
@bafonins
Copy link
Contributor

bafonins commented Aug 26, 2019

The main problem with the current implementation of the device form is that everything is coupled together. This causes

  1. Complex and long validation schema
  2. No straightforward way to disable certain fields based on the stack configuration
  3. No straightforward way to change the form fields/field titles accoring to selected values
  4. Unnecessary complexity in the js sdk for batched creation/update/deletion as well as creation rollback logic

I propose to implement the device form as the multi-step form. This can look something like this:
Screenshot 2019-08-26 at 11 29 16
Screenshot 2019-08-26 at 11 31 57
Screenshot 2019-08-26 at 11 34 16

Such approach addresses all issues mentioned above:

  1. Instead of one complex schema we define a small one for each step.
  2. Simply disable the whole step if a certain reponsible component is not available in the stack. What is more, we can inform the user about this via description/notification.
  3. Adapt each step depending on the previously submitted values. For example, change Join EUI label to App EUI for lorawan version 1.0.x.
  4. No need for batched requests. The user becomes responsible for creating device in different components. However we might want to keep batched deletion for convenience.

Editing devices can be implemented as a stack of accordions:
Screenshot 2019-08-26 at 11 56 36
Where each accordion expands with a standalone form.

Besides the device form we can use wizard for the application form as well to:

  1. Create the application in the is
  2. Link the application to as

cc @kschiffer @johanstokking @htdvisser

@bafonins bafonins added needs/discussion We need to discuss this and removed in progress We're working on it labels Aug 26, 2019
@johanstokking
Copy link
Member

Sounds good to me, especially if we can group fields for components together in a step. That way, we can simply skip/disable steps when a component is not available.

Referencing #1234 also; even if JS is available, the user can skip the JS fields.

@bafonins
Copy link
Contributor

bafonins commented Aug 26, 2019

I came up with such diagram:
device-wizard-diagram

Every node is a step

  • Steps with dotted outline do not submit the form but keep it in the local state for the next steps.
  • Others send requests to the server on submission.

It looks complicated, but it the current implementation has even bigger state space regarding validation/submission/field mask generation/etc.

Consider the diagram as an initial proposition to start the discussion.

@htdvisser
Copy link
Contributor

I think this is a good start.

  • Maybe the flow would be better if the Join Server came first.
  • An important thing we haven't really made any progress on yet is pre-filling fields from device templates in the device repository Set up LoRaWAN Device Repository #263. I think that could really simplify the process for production deployments.

And some small things:

  • A DevEUI is not forbidden for ABP devices (it can optionally be set)
  • LoRaWAN 1.0.x devices don't have a NwkKey, only an AppKey
  • FNwkSIntKey is called NwkSKey (towards the user)
  • The Application Server does not get the NwkKey or AppKey

@johanstokking
Copy link
Member

Yes, great start.

  • Maybe the flow would be better if the Join Server came first.

I agree with this. If the activation mode is OTAA and the cluster JS is enabled, on the JS page users get the choice to enter root keys and store them on the cluster JS. (if they disable that, NS uses JS lookup, just like when there is no JS in the cluster enabled)

@rvolosatovs
Copy link
Contributor

  • "Activation mode" is in payload - in your diagram it corresponds to actually 2 fields - multicast bool and supports_join bool. There are 3 possible options, since multicast && supports_join is not valid.

  • frequency_plan -> frequency_plan_id

  • resets_f_cnt is optional, false by default.

  • FNwkSIntKey should be presented as NwkKey only for <1.1

  • FNwkSIntKey is missing in ABP NS settings for >= 1.1

  • multicast devices only need AppSKey - NS does not need any keys for multicast devices to work, only AS

@bafonins bafonins modified the milestones: Backlog, Next Up Aug 27, 2019
@bafonins
Copy link
Contributor

@htdvisser

Maybe the flow would be better if the Join Server came first.

Why?

An important thing we haven't really made any progress on yet is pre-filling fields from device templates in the device repository #263. I think that could really simplify the process for production deployments.

For me it seems outside of the scope of this issue

A DevEUI is not forbidden for ABP devices (it can optionally be set)

Do we want to set it for ABP devices as well? I would say the fewer fields we have, the better. However, it is needed we can add it.

FNwkSIntKey is called NwkSKey (towards the user)

So the label should be NwkSKey for both 1.0.x and 1.1.x ? Here is what we have atm in the console:
Screenshot 2019-08-27 at 19 43 37

  • LoRaWAN 1.0.x devices don't have a NwkKey, only an AppKey
  • The Application Server does not get the NwkKey or AppKey

Fixed 👌

@johanstokking

If the activation mode is OTAA and the cluster JS is enabled, on the JS page users get the choice to enter root keys and store them on the cluster JS. (if they disable that, NS uses JS lookup, just like when there is no JS in the cluster enabled).

So this is just the matter of allowing users to skip the join server submission step? No requests to js at all?

Regarding #1134, could you also specify where do these fields belong in the diagram?

@rvolosatovs

Activation mode" is in payload - in your diagram it corresponds to actually 2 fields - multicast bool and supports_join bool. There are 3 possible options, since multicast && supports_join is not valid.

Just to clarify:

  1. supports_join=true - OTAA
  2. supports_join=false - ABP
  3. multicast=true && **no** supports_join - multicast
    Is this correct?

resets_f_cnt is optional, false by default.

I think it is still fine to show it to the users. Should allow the users to set it for multicast devices?

FNwkSIntKey should be presented as NwkKey only for <1.1

You mean NwkSKey ?

frequency_plan -> frequency_plan_id
FNwkSIntKey is missing in ABP NS settings for >= 1.1

Fixed 👌

@bafonins
Copy link
Contributor

Updated diagram:

device-wizard-diagram2

@johanstokking
Copy link
Member

johanstokking commented Aug 27, 2019

@htdvisser

Maybe the flow would be better if the Join Server came first.

Why?

Yeah I'm coming back on this; I think it makes more sense to enter MAC versions before entering keys. So I'm supporting the current flow. If MAC version is entered (when NS is enabled), we don't have to ask for NwkKey since that's 1.1.x.

An important thing we haven't really made any progress on yet is pre-filling fields from device templates in the device repository #263. I think that could really simplify the process for production deployments.

For me it seems outside of the scope of this issue

It is out of scope indeed.

A DevEUI is not forbidden for ABP devices (it can optionally be set)

Do we want to set it for ABP devices as well? I would say the fewer fields we have, the better. However, it is needed we can add it.

Yes, we want to optionally ask for it. The more identification information we have about end devices, the better. It is also required in stateful passive roaming. The reason we don't force users to enter the DevEUI, is because if there is no DevEUI, we don't want users to enter bogus values.

FNwkSIntKey is called NwkSKey (towards the user)

So the label should be NwkSKey for both 1.0.x and 1.1.x ? Here is what we have atm in the console:
Screenshot 2019-08-27 at 19 43 37

It's NwkSKey for 1.0.x and FNwkSIntKey for 1.1.x.

If the activation mode is OTAA and the cluster JS is enabled, on the JS page users get the choice to enter root keys and store them on the cluster JS. (if they disable that, NS uses JS lookup, just like when there is no JS in the cluster enabled).

So this is just the matter of allowing users to skip the join server submission step? No requests to js at all?

Indeed.

An example is a device featuring Semtech's modem which uses the Semtech Join Server. We only need to know EUIs in that case.

Regarding #1134, could you also specify where do these fields belong in the diagram?

They belong in the JS step; these fields to go JS, if the JS is enabled in the cluster and if the user wants to provision the devices on the JS.

These fields are optional.

Just to clarify:

  1. supports_join=true - OTAA
  2. supports_join=false - ABP
  3. multicast=true && **no** supports_join - multicast
    Is this correct?

Strictly:

  1. OTAA: supports_join
  2. ABP: !supports_join && !multicast
  3. Multicast: multicast

Invalid is supports_join && multicast, but this is (or should be) validated at NS.

resets_f_cnt is optional, false by default.

I think it is still fine to show it to the users. Should allow the users to set it for multicast devices?

No, this is not for multicast. resets_f_cnt refers to uplink, but there ain't uplink in multicast.

FNwkSIntKey should be presented as NwkKey only for <1.1

You mean NwkSKey ?

Should be NwkSKey indeed.

@rvolosatovs
Copy link
Contributor

@bafonins please see @johanstokking 's answer.
In regards to multicast - device address is required as well

@bafonins
Copy link
Contributor

bafonins commented Aug 28, 2019

@rvolosatovs
Copy link
Contributor

Device Address still missing from multicast device Network Server settings

@bafonins
Copy link
Contributor

Device Address still missing from multicast device Network Server settings

Updated 👌

@johanstokking
Copy link
Member

Multicast can be 1.0.x and 1.1.x. Entering the session information (DevAddr and keys) is the same for multicast as for ABP.

@kschiffer
Copy link
Member Author

@bafonins can you give a status update and timeline on this issue?

@htdvisser htdvisser modified the milestones: April 2020, May 2020 May 1, 2020
@bafonins bafonins removed the blocking Another issue or pull request is waiting for this label May 11, 2020
@johanstokking
Copy link
Member

For app_s_key and skip_payload_crypto, here's the thing;

  • AS respects the field skip_payload_crypto of the end device. If true, AS does not perform payload encryption and decryption
    • AS will get a skip_payload_crypto on the application-level too (via Link, like payload formatters), which takes precedence over the end device's setting
  • There's probably always a app_s_key in AS, but it may be wrapped, i.e. session.keys.app_s_key.key is not set (but encrypted_key and kek_label is set)
    • When AS doesn't have the KEK, i.e. it can't unwrap the encrypted key. Now, that errors. We will probably just return the encrypted app_s_key as is to clients
  • In the Console, if skip_payload_crypto is set true (on end device or application link), don't bother with app_s_key: disable it, don't care

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/console This is related to the Console in progress We're working on it ui/web This is related to a web interface
Projects
None yet
Development

No branches or pull requests

5 participants