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

Flasher and hierarchical config manager #110

Open
wants to merge 98 commits into
base: master
Choose a base branch
from

Conversation

r41d
Copy link

@r41d r41d commented May 22, 2023

This adds the possibility to flash and configure multiple boards with varying configs.
Configurations can be created in a tree-like structure and each (except the root) inherit values from their parent and can override certain fields.
For each config it can then be specified how many boards shall be given the specific config, an example would be to have a couple of nodes with Bluetooth enabled and several with GPS configured. Setting the LoRa region in the root config is probably a good idea.
Slight modifications on the js package are needed, hence the depency currently points to this fork.

Feedback appreaciated

@CLAassistant
Copy link

CLAassistant commented May 22, 2023

CLA assistant check
All committers have signed the CLA.

@sachaw
Copy link
Member

sachaw commented May 22, 2023

Exceptional work, I'll work through reviewing this tomorrow.
As A start we should focus on merging the fixes in the js library.

@sachaw
Copy link
Member

sachaw commented May 22, 2023

Another focus should be to get preview deployments building (broken package lock atm)

@rcarteraz
Copy link
Contributor

Just built this locally to check out and wow! This is very impressive! I have some feedback but it might be a little premature for that, so I'll hold off for now. I'm very excited for what comes out of this though. Great job so far!

Comment on lines -67 to -71
disabledBy: [
{
fieldName: "usePreset"
}
],
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind having all options enabled?

@sachaw
Copy link
Member

sachaw commented May 23, 2023

@rcarteraz happy for any feedback

@rcarteraz
Copy link
Contributor

@rcarteraz happy for any feedback

Sounds good. I'm out right now but I'll post when I get back this evening.

@rcarteraz
Copy link
Contributor

First, I really like this can't compliment the work that's been done enough, because it's fantastic! I think it's a wonderful update that will be immensely useful, but maybe with some changes first. Purely thinking about the user experience here, which is why I bring these items up. Okay, so a couple items I can think of.

  • This new version opens straight to the combined flasher/configure screen. I think this would be very overwhelming to newer users. We should keep the initial connection flow we currently have and make this an option on the sidebar. Either of these two areas below. Another reason for this would be the fact that the flasher (at least as far as I can tell) is still for ESP32 devices only (which is also something we should denote). This way (assuming I'm correct) we don't lead users to believe the can use it with their device when they cannot.
Screenshot 2023-05-23 at 11 03 42 PM
  • Additionally, I think we should create a toggle and have two views here. Maybe "Standard" and "Advanced". Standard should be just the flashing elements. Again, the reason for this is it can be overwhelming to some users, who are unsure if they need to select any of those options or not. Plus if they're just updating the firmware, this keeps them from making any changes to their current setup. For those that want to flash/configure multiple devices they can flip the toggle and easily do so. And for the rest they can configure as they have.
Screenshot 2023-05-23 at 10 55 59 PM
  • Might be a good idea to remove auto-detect. It's a little wonky and likely will cause more issues than not. We should just have the users select their device manually. Unless we can improve this somehow.
  • There doesn't seem to be an option to just update the firmware. Even if you do not toggle "Force Wipe and Reinstall" it seems to do exactly that still. At least in the testing that I've done so far.

I think that's all I have for now. It's late so I'll leave it at this and if anything else comes to mind, I'll be sure to add it.

@thebentern
Copy link
Contributor

thebentern commented May 25, 2023

  • Might be a good idea to remove auto-detect. It's a little wonky and likely will cause more issues than not. We should just have the users select their device manually. Unless we can improve this somehow.

Auto-detect for updates should be doable if a want-config is called and we get the HWModel from the DeviceMetadata payload.

@garthvh
Copy link
Member

garthvh commented May 25, 2023

Auto detect is pretty much impossible if the device does not have Meshtastic on it yet so a manual path is always needed devices with Meshtastic can be identified as @thebentern stated.

@garthvh
Copy link
Member

garthvh commented Jun 28, 2023

Is there a risk to any existing functionality by merging this? Seems like great features separate from the existing functionally that we can patch as we find stuff. The pull is starting to get stale which is never any fun for a large pull.

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

8 participants