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

Add initial doc for imperial #7

Merged
merged 14 commits into from
Nov 10, 2021
Merged

Add initial doc for imperial #7

merged 14 commits into from
Nov 10, 2021

Conversation

jesserockz
Copy link
Member

No description provided.

@jesserockz
Copy link
Member Author

@Aircoookie @OttoWinter do you have any thoughts or feedback on this proposal?

src/imperial.md Outdated
| ------ | ---------------------- | ------------------------------------------------ |
| `0x02` | Ready (Authorized) | Ready to accept credentials. |
| `0x03` | Provisioning | Credentials received, attempt to connect. |
| `0x04` | Provisioned | Connection successful. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it's this state, we should allow the device to specify a url that the client can let the user visit to interact with the device, similar to the URL returned from "Send Wi-Fi settings"

src/imperial.md Outdated Show resolved Hide resolved
@Aircoookie
Copy link

Hey!

@jesserockz Thank you for creating a doc as well :)

I had made a proposal for ImprovSerial a few weeks ago, now revised it a bit with feedback from @balloob in discord to my first version.
I would strongly suggest using JSON as the data format, as it is a lot easier to both generate and parse in JS/TS and (if using a library like ArduinoJSON) on the host. A byte map format, as you are proposing, is easier without a C JSON library I have to admit, but it is also possible to generate and parse responses with std c string functionality.

However, I agree about leaving the states aligned with those in Improv (BLE) to a large extent, I have also kept them in my proposal: https://gist.github.com/Aircoookie/203ade91cd89e2041939c766c1257ffb

Of course I'd be open to implementing your proposal to WLED as well, however I believe using JSON would give the protocol a great deal of additional flexibility for extra values and extensions (at the cost of a slightly more difficult implementation, at least without external libraries)

@balloob
Copy link
Contributor

balloob commented Aug 9, 2021

I initially leaned into JSON too, but after consulting with various people concluded that it will hurt portability because people might need to pull in a JSON lib just for us – and it will prevent us from creating a reusable SDK.

@Aircoookie
Copy link

I initially leaned into JSON too, but after consulting with various people concluded that it will hurt portability because people might need to pull in a JSON lib just for us – and it will prevent us from creating a reusable SDK.

That is a valid concern indeed - the JSON would need to be kept simple to enable easy parsing and creation even without a lib.
I could definitely give implementing a dependencyless Reference Library for us a shoot, which would parse the (relevant) keys in the JSON and assemble response strings solely using standard C str functions.

If you prefer the byte packet approach however, I can 100% understand why, that will result in a more barebones and easier to understand reference library.
I'm just worried that this could limit the scope of application for imperial to only setting ssid + pass. Another option would be adding an RPC command for sending JSON and essentially building another optional protocol on top, not required at all for basic functionality. This would make the whole protocol needlessly complicated though (at least in the non-trivial case user-defined values/settings are desired)

@balloob
Copy link
Contributor

balloob commented Aug 10, 2021

Can you elaborate on your use case. What extra information do you think Imperial would get in the future ?

(Reminds me, we might want to add a version int to the initial payload from server)

@Aircoookie
Copy link

Version should definitely be included, yeah.

My personal use-case would be wifi + pass + a device name + maybe static IP configuration down the line (even though static IP is an advanced feature many will not need - and if they want it they can set it via the normal webinterface) and returning some values upon successful connection (URL for sure, but maybe also WiFi signal strength)

However, the point I want to make is that what matters is not my personal use case but the assumption that this protocol should be useful to a lot of users with different use-cases.
It seems like we might not be on the same page here on what we want this protocol to be (even though the core use cases align)

  1. @jesserockz 's proposal: Do WiFi provisioning and that only (for now at least), as simple and lightweight as possible.
  2. My proposal: Make a protocol for general provisioning of Arduino/C devices over Serial. That may or may not include wifi provisioning. For example, it could also be used to configure a simple Arduino over Serial (where updating configuration would require a firmware reinstall otherwise). Imperial just provides a framework for this information exchange.
    A high-level summary of the initial handshake is:
  • Host -> Device: Do you run imperial?
  • Dev -> Host: Yes, my project is WLED and the version 0.13.0
  • Host -> Device: Great, I know how to provision you. Here are the values I know your project expects

Basically, the objectives of these two proposals are quite different. My proposal offers more functionality and flexibility at the cost of implementation difficulty. Since I'm personally, currently only looking for WiFi provisioning, I'd be just as happy to see @jesserockz 's proposal realized personally, I just believe my version could help out a lot of potential users with different use cases than me (which may or may not include future me :) )

@OttoWinter
Copy link

Yeah I also see the value in the added functionality JSON would offer.

Initially I was against it (for the reasons stated above regarding libraries). But after some more thought maybe that's not a huge issue. All projects I looked through (Tasmota, ESPEasy, WLED, and of course ESPHome) have some JSON library already included - especially for WiFi chips that this is targeted:

  1. most projects will already have JSON lib (for any HTTP API, or even communication with their backend)
  2. any wifi device will have enough resources anyway to add a json lib

There are only a couple major JSON libs I see used anyway (ArduinoJson - covers all Arduino-based DIY projects, cJson for esp-idf based ones), which shouldn't be that hard to support for us.

Even if a project uses a different lib, if we make the protocol simple enough it won't be hard to integrate anyway (most more professional projects will do this anyway I think, because adding libs to other build setups is often difficult)

@OttoWinter
Copy link

Copying two other things here from chat with jesse:

  1. There needs to be some way to detect if a device supports imperial. Like a status request to check if imperial is supported (then we can display a message like this device isn't supported, instead of a generic error)
  2. We should recommend a baud rate for the serial port - otherwise if any device uses a different baud rate, other imperial client implementation won't be able to talk to it. We probably can't mandate it though, because having to change baud rate is really something that I think is a barrier to entry for projects. Our default client implementation could just scan through a bunch of common baud rates and check for each if it detects a device (with 1.)

@OttoWinter
Copy link

Re JSON: maybe there's a good compromise:

We add an additional data (raw bytes) field to the existing proposal where clients can write arbitrary strings. A device can then choose to interpret this any way it wants, for example as JSON. Any imperial client implementation would then work with any device, but we also get the flexibility of additional fields

@Aircoookie
Copy link

@OttoWinter

Of course, support check is important. In my proposal, state 1 is sent by the client to request information from the device (that is, imperial support in the first place, version, and maybe additional info/requirements). I would propose integrating a similar capability into the current proposal as well. As for devices without Imperial support a timeout in which no (valid) response is received is likely the only way to detect a device that does not support imperial.

On baudrate, I'd propose 115200 as the default, it is the best middle ground between widespread support, acceptable speed and good reliability.

Yeah, perhaps not forcing JSON upon every potential user of the protocol, but allowing it optionally, would be a good idea. I would propose a concept of "provisioning schemes", e.g. scheme 1 would be byte-format SSID + password, scheme 2 (e.g.) SSID + password + static IP, scheme 3 SSID + password + device name, scheme 4 a user-defined JSON string.

On state 1 (client checking device imperial support), the device would return a list of the provisioning schemes supported by it.
Alternatively, it could return a string identifying the device type/project/software, and the client can pick the most appropiate scheme for that - or if unknown, fallback to scheme 1 as a baseline.

@Aircoookie
Copy link

One additional benefit of JSON I failed to mention so far: It is 100% human readable/editable. Thinking back to the Arduino example, you could provision it manually using the serial console, which would become impractical once non-ascii bytes (flags, length fields) come into play.

@balloob
Copy link
Contributor

balloob commented Aug 10, 2021

Here are the Improv Scope & Constraints from the homepage:

The goal of the standard is to get the device connected to the Wi-Fi via Bluetooth Low Energy (BLE). It is not the goal to offer a way for devices to share data or control. The standard should work without requiring the device to contain a screen.

I am not sure if anything beyond getting the device connected should be part of the scope of Imperial. The more daunting we make it, the more resistance we might get from projects to adopt it. And the goal is adoption.

I also am not sure if we should aim to be extensible so people can build protocols on top. Wouldn't it be better to suggest those projects to make a new protocol instead, with total freedom? Or just have them redirect to the device website, that can offer anything else.

I think that this is the ideal flow:

  1. Connect to computer
  2. Open website with Imperial Configurator or web flasher with imperial support
  3. Let device connect to Wi-Fi
  4. Be forwarded to device to finish onboarding/edit settings including any advanced settings.

@Aircoookie
Copy link

@balloob You have changed my mind.

Adoption is indeed important, and my idea about how the client would need to know the requirements of the device in order to be able to provision it would not be required. We cannot reasonably mandate that someone who choses to use imperial to provision their devices to have control over and/or add support for their device to the imperial client to be used (e.g. web flasher)
Additionally, the more lightweight the library/SDK is, the more likely it will be adopted. Parsing and generating JSON without external libraries would likely need somewhere 256B~1kB of RAM mostly for string constants, which would be neglegible on an ESP32, but much less on an 8266.

Therefore, I agree with the idea of keeping Imperial WiFi-provisioning only. Every other case can be either handled by the device itself or by a different serial protocol :)

To summarize, the only two minor things that definitely should still be added to @jesserockz 's proposal are a version byte and a way to return the URL string (not sure if this is already in the proposal as e.g. the first string returned by a RPC response type 0x04 packet)

src/imperial.md Outdated Show resolved Hide resolved
@balloob
Copy link
Contributor

balloob commented Aug 24, 2021

This is looking good and seems workable.

@Aircoookie
Copy link

Approving of the current revision too.

@@ -1,6 +1,6 @@
---
layout: base
title: Documentation
Copy link
Contributor

@balloob balloob Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update the filename too, and add a Netlify redirect so the old URL remains working. Do that by creating a _redirects file like this in the public folder https://github.com/home-assistant/home-assistant.io/blob/current/source/_redirects

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this exact thought..haha

@@ -3,7 +3,9 @@
- - /
- Home
- - /documentation/
- Documentation
- BLE Documentation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fit in the header. Maybe just do BLE ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change to just BLE and Serial

@balloob balloob merged commit 5c49cd8 into main Nov 10, 2021
@balloob balloob deleted the imperial branch November 10, 2021 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants