Skip to content

Conversation

@ZigiMigi
Copy link
Contributor

@ZigiMigi ZigiMigi commented Feb 23, 2022

Moved GUI from Flash boot speedup branch to Develop branch.

Moved GUI from Flash boot speedup branch to develop branch.
Copy link
Collaborator

@themarpe themarpe left a comment

Choose a reason for hiding this comment

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

TODO:

  • Combine files into single file and move to utilities/device_manager.py

  • Somewhere in the GUI or in "About", print current version & commit if available (& maybe branch, by a lookup) of the depthai library. (Accessible using: dai.__version__ & dai.__commit__)

  • Add capability to flash depthai application package (dap), created by createDepthaiApplicationPackage and flashed by flashDepthaiApplicationPackage

  • ... (in the future, we'd have to expose the capability to Python) Add SBR viewer (like: https://github.com/luxonis/SBR/blob/main/examples/sbr-util/README.md), to view .dap files (and or read out existing on device flashed apps...)

def getDevices(window, devices):
listedDevices = []
devices.clear()
deviceInfos = dai.Device.getAllAvailableDevices()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use dai.XLinkConnection.getAllConnectedDevices() and print the state along them, just so this also serves as listing all devices (no matter the state)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE:

  • renamed file and moved to ustilities/device_manager.py (everything is now in 1 file)
  • GUI now has 2 pages (About and Configuration settings)
  • GUI also informs dai.version and dai.commmit
  • Added button Flash DAP that runs flashDepthaiApplicationPackage function
  • Changed device finding from dai.Device.getAllAvailableDevices() to dai.XLinkConnection.getAllConnectedDevices()
    That is everything?

- renamed file and moved to ```ustilities/device_manager.py``` (everything is now in 1 file)
- GUI now has 2 pages (```About``` and ```Configuration settings```)
- GUI also informs dai.__version__ and dai.__commmit__
- Added button ```Flash DAP``` that runs ```flashDepthaiApplicationPackage``` function
- Changed device finding from ```dai.Device.getAllAvailableDevices()``` to ```dai.XLinkConnection.getAllConnectedDevices()```
Moved buttons for moving between pages to menu bar.
Added pressed animation to menu buttons.
Changed button "About" to "Device select" to not confuse users
@ZigiMigi ZigiMigi requested a review from themarpe February 24, 2022 18:41
@themarpe
Copy link
Collaborator

themarpe commented Mar 1, 2022

@ZigiMigi
Please enable changes from maintainers to your PR branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Have some tweaks to be added.

As for the rest - I don't seem to see the whole GUI - missing Flash DAP, ...

image

The GUI is Device Manager and should not present Bootloader only information (I've made some tweaks on this accord, I'll push once you enable changes)

Current version & commit are library level information - move those to another tab Library or the likes. (Eg. they aren't related to selected device...)

Please update #517 to add upon this changes

@ZigiMigi
Copy link
Contributor Author

ZigiMigi commented Mar 1, 2022

@themarpe

  • Changes are enabled
  • As for the screenshot that you sent, hmm you should have 3 buttons at the bottom on this tab (Flash config, Flash DAP and Flash clear) --> this might be because the window is in a specific dimension (linux window might be a little bigger than the windows, which would be the cause of linux not having buttons at the bottom) FIX: resizing the window
  • Commit and Version will be added to another tab, should i add anything else to that tab (so that it won't be so empty)?
  • Issue Flash boot speedup GUI additions #517 (What exactly should i do there? Do i close that issue?)

@themarpe
Copy link
Collaborator

themarpe commented Mar 2, 2022

Thanks @ZigiMigi - I've pushed my changes.

Commit and Version will be added to another tab, should i add anything else to that tab (so that it won't be so empty)?

Thats fine for now.

Issue #517 (What exactly should i do there? Do i close that issue?)

Just rebase the changes on top of current work (eg. so one can merge them into this branch later) - basically to bring that branch up to date, be a single file, as well as changes we are discussing and ones I've just pushed...

Not needed immediately, I though I'd need it sooner, so it can wait until this PR gets merged first... (but will be required then, to be able to merge)

@ZigiMigi
Copy link
Contributor Author

ZigiMigi commented Mar 2, 2022

@themarpe
I've viewed your changes.

# TODO - might be better to readConfig instead of readConfigData
 conf = bl.readConfigData()

This was the idea in the first place to use just readConfig, but in my testing readConfig always gave me default values and not values that were changed (if I maunaly changed mac address to "FF:FF:FF:FF:FF:FF", readConfig returned ":::::", while readConfigData returned "FF:FF:FF:FF:FF:FF" )

 # TODO - Don't modify the device without explicit action
    # if conf is None:
    #     success, error = bl.flashConfig(dai.DeviceBootloader.Config())

Above modification is also "needed", if Factory reset is used on a POE device it sets its config data to None and that becomes a problem in the GUI as you can not read values from None

As for issue #517, I'll bring it up to date today.

@themarpe
Copy link
Collaborator

themarpe commented Mar 2, 2022

This was the idea in the first place to use just readConfig, but in my testing readConfig always gave me default values and not values that were changed (if I maunaly changed mac address to "FF:FF:FF:FF:FF:FF", readConfig returned ":::::", while readConfigData returned "FF:FF:FF:FF:FF:FF" )

Sounds like a bug - I'll take a look later today...

Above modification is also "needed", if Factory reset is used on a POE device it sets its config data to None and that becomes a problem in the GUI as you can not read values from None

Problem is that devices can also not have flash storage, to be able to even flash the config... (eg. OAK-D). So try testing with those devices as well. Not a lot of functionality to test there, but should still work.

As for issue #517, I'll bring it up to date today.

Thanks!

@ZigiMigi
Copy link
Contributor Author

ZigiMigi commented Mar 2, 2022

Problem is that devices can also not have flash storage, to be able to even flash the config... (eg. OAK-D). So try testing with those devices as well. Not a lot of functionality to test there, but should still work.
Problem only occurs with POE devices from my tests. When i tested OAK-D-IOT, I had no problems. I think this can be solved by prompting the user, about the above mentioned flash, and if the ansewr is no, just output some basic values to the GUI.

@themarpe
Copy link
Collaborator

themarpe commented Mar 2, 2022

When i tested OAK-D-IOT, I had no problems. I think this can be solved by prompting the user, about the above mentioned flash, and if the ansewr is no, just output some basic values to the GUI.

OAK-D-IOT does have flash.

You should properly handle a "null" config as its a valid state. Config can be either:

  • Not flashed
  • Flashed to default values
  • Flashed to custom values

@ZigiMigi
Copy link
Contributor Author

ZigiMigi commented Mar 2, 2022

Small correction, when i was playing around with IOT I flashed it like you would an POE device and i somehow deleted it's configs to None.
But yes I think that this needs to be added when reading values (if its None, output a default value or a "None" value).

@themarpe
Copy link
Collaborator

themarpe commented Mar 2, 2022

@ZigiMigi

Please add the following capability as well:

// DeviceBootloader.hpp
    /**
     * Boots into integrated ROM bootloader in USB mode
     * @throws A runtime exception if there are any communication issues
     */
    void bootUsbRomBootloader();

Should fit under first tab as a button

Added new button "Flash from usb"
@ZigiMigi
Copy link
Contributor Author

ZigiMigi commented Mar 2, 2022

New button added Boot from USB (hope the name is ok, if not I will change it)
I also added a few comments (I can add more of them, to better explain the code?)

Copy link
Collaborator

@themarpe themarpe left a comment

Choose a reason for hiding this comment

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

All functions like getConfigs, flashBootloader, flashConfig, ... could use same "device" after its "Selected".
(As it otherwise causes long delays in booting up for each operation)

window['Factory reset'].update(disabled=False)
# window['Reset configuration'].update(disabled=False)
window['Flash DAP'].update(disabled=False)
window['Boot from USB'].update(disabled=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to Boot into USB Recovery mode



def getConfigs(window, device, devType):
bl = dai.DeviceBootloader(device)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only connect/disconnect to a device when needed

@themarpe
Copy link
Collaborator

themarpe commented Mar 2, 2022

I can add more of them, to better explain the code?

Rarely a bad idea, especially in ambiguous situations

Changed program so that you connect to the device only once
@ZigiMigi
Copy link
Contributor Author

ZigiMigi commented Mar 3, 2022

@themarpe

  • Changed Boot from USB to Boot into USB Recovery mode
  • Changed functions so that they get bl = dai.DeviceBootloader(devices[values['devices']]) as an argument (connecting to the device is now only once and not in every function separately)

@themarpe themarpe requested a review from Erol444 March 3, 2022 16:15
@themarpe
Copy link
Collaborator

themarpe commented Mar 3, 2022

@Erol444 Please test this out on your end as well and comment if any additional changes are required (as you see fit)

@themarpe
Copy link
Collaborator

themarpe commented Mar 3, 2022

@ZigiMigi
Also add a requirements.txt file along in the utilities folder, that install pysimplegui.
There is also a need for python3-tk package, that is usually installed separately (atleast on Ubuntu)

Add a try catch for importing packages and issue a meaningful message that tells to install the requirements.txt & python3-tk package as

print(f'Make sure requirements are installed: ')
print(f'$ {sys.executable} -m pip -r requirements.txt')
print(f'And if running Linux/Ubuntu, make sure python3-tk package is installed')
print(f'$ sudo apt install python3-tk')

Added install_requirements.py and requirements.txt
@ZigiMigi
Copy link
Contributor Author

ZigiMigi commented Mar 4, 2022

@themarpe

  • Added requrements.txt
  • And added intall_requirements.py that installs requirements.txt and if user is on linux also runs print(f'$ sudo apt install python3-tk')

Comment on lines +1 to +20
import subprocess
import sys


in_venv = getattr(sys, "real_prefix", getattr(sys, "base_prefix", sys.prefix)) != sys.prefix
pip_call = [sys.executable, "-m", "pip"]
pip_install = pip_call + ["install"]

if not in_venv:
pip_install.append("--user")

subprocess.check_call([*pip_install, "pip", "-U"])
subprocess.check_call([*pip_call, "uninstall", "depthai", "--yes"])
subprocess.check_call([*pip_install, "-r", "requirements.txt"])
try:
subprocess.check_call([*pip_install, "-r", "requirements-optional.txt"])
if sys.platform == "linux":
print(f'$ sudo apt install python3-tk')
except subprocess.CalledProcessError as ex:
print(f"Optional dependencies were not installed (exit code {ex.returncode})") No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd install python3-tk in this case...
Have you tested this. This seems like it just uninstalls the depthai as of right now...
Also requirements-optional.txt aren't available...

Anyway, in this case I think best thing would be moving the examples/install_requirements.py down to /install_requirements.py and adding an argument IFF only specific requirements are needed (eg docs/utilities/examples, although that can be added later). Otherwise same principle applies - installs depthai, dependencies, etc...

CC: @Erol444 @szabi-luxonis on above

Copy link
Contributor

@Erol444 Erol444 left a comment

Choose a reason for hiding this comment

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

  • When you open up the app, it should automatically perform Search and auto-select the first device it sees.
  • When you select a device with drop-down, it should automatically select it, without the need to click Select button (so remove that button)
  • Yellow/White color scheme for buttons at the bottom has to be improved, as text can't be read easily.
  • Using latest depthai version (2.15), I get an error when flashing bootloader
line 185, in flashBootloader
    bl.flashBootloader(progress)
ValueError: DeviceBootloader wasn't initialized to allow flashing bootloader. Set 'allowFlashingBootloader' in constructor
  • Why is depthai version text underlined? Current __version__:
  • When flashing bootlaoder/app, maybe indicate the callback
  • Under Current bootloader version, there's an issue as it's always stated None, even after flashing the new bootloader and restarting the app
  • Background could be a bit lighter, so the black text is easier to read
  • When you click on Config, it should be written which device are you setting the Config for. It should also throw an error if you try to set network related configs to the USB device (not sure if implemented).

Fixed requested changes and also made GUI flashing to work so that you may flash only wanted configs and not all.
@ZigiMigi
Copy link
Contributor Author

Addressed needed changes:

  • One problem with searching and selecting without buttons is that, the GUI needs a triggered event to changes things in the window, which means that the buttons are needed, as if you call a function to seach devices before any event is triggerd nothing will happen and the program will wait for an event...
  • Button colors have been changed to orange (now the text in buttons is visible much better)
  • Fixed flashing problem bl = dai.DeviceBootloader(devices[values['devices']]) needs to be bl = dai.DeviceBootloader(devices[values['devices']], True)
  • Changed __version__ to current Depthai version for better understanding
  • I do not know what kind of callback is needed, the user is already informed if flashing is successesful?
  • Background changed from dark grey to light grey (much easier to read now)
  • The device name is always displayed on the About device tab, i dont know where exactly to put name of the device on Configuration tab (already a lot of information there)
  • ALSO added so that not all configs are needed for flashing configuration (you may enter only wanted values)

Buttons select removed, GUI works with combo' events now( clicking in the combo box triggers same event as select did).
Changed window size and added device name to config tab.
Fixed values error
Copy link
Contributor

@Erol444 Erol444 left a comment

Choose a reason for hiding this comment

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

LGTM, we should merge into master branch.

Copy link
Collaborator

@themarpe themarpe left a comment

Choose a reason for hiding this comment

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

I have left my reviews & fixme notes in the commit I've pushed.

Overview of TODO

  • Bootloader type as an entry in about (USB/NETWORK)
  • Search for devices upon starting the program (reduces 1 click for UX reasons - and its a non-destrcutrive operation and doesn't require to be explicit)
  • Allow flashing of different type of bootloaders (maybe under a separate tab as "advanced")
  • Initially allowFlashingBootloader = False & later when BL wants to be flashed reconnect (reason: if that option is set the device is automatically booted with latest BL in library, also it takes much longer to connect)
  • Test with a device flashed with an older bootloader
  • Also connecting to a "BOOTED" devices doesn't error out properly AFAIK (eg one device running some example already, etc...)

Fixed addressed changes.
@ZigiMigi
Copy link
Contributor Author

Addressed above changes and requested fixes:

  • Bootloader type is now an entry (when flashing bootloader, type will be taken from this combo),
  • Upon starting the GUI, search is performed,
  • Allow flashing of different type of bootloaders (maybe under a separate tab as "advanced") not exactly sure what should be in that tab?
  • Initialy allowFlashingBootloader is set to FALSE, when needed (when flashing) device will be reconected with allowFlashingBootloader to TRUE,
  • Connecting to a BOOTED device now promts out a message.

ALSO:

  • added try/catch to every major function.

@themarpe
Copy link
Collaborator

Flashing different types of bootloads should give you an option to flash either USB or NETWORK bootloader. By default, it flashes the one that matches the type of connection to the device.

@ZigiMigi
Copy link
Contributor Author

But isn't that already achived with the combobox Bootloader type ?

@themarpe
Copy link
Collaborator

@ZigiMigi rechecked - looks good. Didn't saw that you've added it in.

Retested with unflashed device, seems like there is still no differentiation between no flashed bootloader and flashed bootloader

# if bl.isEmbeddedVersion() is True:
#     window.Element('currBoot').update('None')
# else:
#     window.Element('currBoot').update(bl.getVersion())

The "isEmbeddedVersion" tells you whether BL had to be booted, or we connected to an already flashed Bootloader.

@themarpe
Copy link
Collaborator

Also, does default still do the expected (if no Bootloader Type is selected)? (Eg. does it flash the USB if its USB and ETHERNET if its a PoE device)?

It might also make sense to move potentially bricking operations to a separate tab, and issue a warning upon entering of that menu, that changing this could soft-brick a device...
Feel free to brainstorm possible actions that could be moved and means of UX of how to issue a warning for users

@ZigiMigi
Copy link
Contributor Author

  • isEmbeddedVersion I will uncomment those lines and add a comment above them ( i put them in comments as i forgot what exactly it did.

  • I am adding a default option to the combobox at the moment that will do that.

Added a default option which will auto select bootloader type based on device type
Addressed requested changes.
@Erol444
Copy link
Contributor

Erol444 commented May 10, 2022

New PR: #579

@Erol444 Erol444 closed this May 10, 2022
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.

3 participants