UI/UX improvements

[PLangowski]

Dasharo/dasharo-issues#1080

[m-iwanicki]

@PLangowski one thing I noticed that in case of error_exit your clear will possibly clear everything along with error messages so user might not know what happened. It needs to be tested.

[PLangowski]

@m-iwanicki From what I see the screen is cleared only after the user presses a key, so they have time to read the error message

[m-iwanicki]

@PLangowski I don't see any waiting for key press in case of error and there are plenty of places where error_exit, error_check or even exit is used

error_exit() {
  _error_msg="$1"
  if [ -n "$_error_msg" ]; then
    # Avoid printing empty line if no message was passed
    print_error "$_error_msg"
  fi
  fum_exit
  exit 1
}

error_check() {
  _error_code=$?
  _error_msg="$1"
  [ "$_error_code" -ne 0 ] && error_exit "$_error_msg : ($_error_code)"
}

[m-iwanicki]

Though maybe if it's called as another script then there are some guards against this, but there are lot of places to check so I'm not 100% sure

[PLangowski]

I tested this and it seems that the functions that use error_exit are called from other scripts. So exit 1 exists only prints the error message and exists from the function. Then, the script that called it waits for a key to be pressed.

[m-iwanicki]

@PLangowski there is functions used e.g. in main_menu_options that might error exit that is compare_versions which is used in couple of places. And I saw it failing couple of times on real hardware (usually when only DPP version is available but credentials are wrong)

[m-iwanicki]

@PLangowski quick test:

root@DasharoToolsSuite:~# diff -u2 dts-functions.sh $(which dts-functions.sh)
--- dts-functions.sh    2024-11-14 13:53:51.007046394 +0000
+++ /usr/sbin/dts-functions.sh  2024-11-14 13:54:57.440023233 +0000
@@ -1516,4 +1516,5 @@
           ;;
           *)
+          compare_versions "1.2.2" ""
           export SEND_LOGS="false"
           echo "Logs will be saved in root directory."

root@DasharoToolsSuite:~# dts

 Dasharo Tools Suite Script 2.0.0
 (c) Dasharo <[email protected]>
 Report issues at: https://github.com/Dasharo/dasharo-issues
*********************************************************
**                HARDWARE INFORMATION
*********************************************************
**    System Inf.: Micro-Star International Co., Ltd. MS-7D25
** Baseboard Inf.: Micro-Star International Co., Ltd. PRO Z690-A WIFI DDR4(MS-7D25)
**       CPU Inf.: 12th Gen Intel(R) Core(TM) i5-12600K
**    RAM Channel-0-DIMM-0: KF3600C17D4/8GX
**    RAM Channel-0-DIMM-1: KF3600C17D4/8GX
**    RAM Channel-0-DIMM-0: KF3600C17D4/8GX
**    RAM Channel-0-DIMM-1: KF3600C17D4/8GX
*********************************************************
**                FIRMWARE INFORMATION
*********************************************************
**      BIOS Inf.: 3mdeb Dasharo (coreboot+UEFI) v1.1.3
*********************************************************
**    SSH status: ON IP: 192.168.10.72/24
*********************************************************
**     1) Dasharo HCL report
**     2) Update Dasharo Firmware
**     3) Restore firmware from Dasharo HCL report
**     4) Load your DPP keys
*********************************************************
R to reboot  P to poweroff  S to enter shell
K to stop SSH server  L to enable sending DTS logs V to enable verbose mode
Enter an option:
1

Please note that the report is not anonymous, but we will use it only for
backup and future improvement of the Dasharo product. Every log is encrypted
and sent over HTTPS, so security is assured.
If you still have doubts, you can skip HCL report generation.

What is inside the HCL report? We gather information about:
  - PCI, Super I/O, GPIO, EC, audio, and Intel configuration,
  - MSRs, CMOS NVRAM, CPU info, DIMMs, state of touchpad, SMBIOS and ACPI tables,
  - Decoded BIOS information, full firmware image backup, kernel dmesg,
  - IO ports, input bus types, and topology - including I2C and USB,

You can find more info about HCL in docs.dasharo.com/glossary
Do you want to support Dasharo development by sending us logs with your hardware configuration? [N/y] n
ERROR Invalid version ''
Incorrect version format
root@DasharoToolsSuite:~#

I'm not sure but it's possible it'd exit whole dts script so maybe it's not a problem that happens as we would have seen it before

[m-iwanicki]

And now that I'm testing it more I'm not sure I like accepting without enter.
If by mistake you press 2 keys then second press will be read on the next read e.g. 5 minutes later after update completes or fails.
More people would need to test it but I think it's easier to make mistakes now.

[m-iwanicki]

@PLangowski If we accept any key then maybe we should clear stdin buffer before reading in case user pressed multiple keys earlier on accident.
Or just change to only accept enter and fix message that informs user about this.

[PLangowski]

From what I see you can't really flush the input buffer in bash. I will change it back to accepting only enter

[PLangowski]

2944250

[m-iwanicki]

From what I see you can't really flush the input buffer in bash.

You can probably read up to e.g. 1'000'000 letters without blocking, that would probably clear the buffer. I think we sometimes use something similar in osfv tests.
But enter is probably better choice

There is still case of compare_versions which can error out. If that happens script exits after which it is started again from beginning. Something should probably be done. Maybe use trap to catch error exit (in scripts/dts) and wait for user input/enter key/sleep for few seconds?

[PLangowski]

does it exit from scripts/dts? Why is it started again?

[m-iwanicki]

It exits current script. There are couple places where compare_versions is used in dts script so it would exit it if it failed.

Why is it started again?

https://github.com/Dasharo/meta-dts/blob/6818b595c8a7f0930d693f8bdf9091c8833bd90b/meta-dts-distro/recipes-dts/images/dts-base-image.inc#L31

[PLangowski]

7984afa
Is this what you meant?

[m-iwanicki]

@PLangowski yes, should probably work. Maybe wait for user input on all error codes, otherwise it's ok