search

siemens / meta-iot2050

SIMATIC IOT2050 Isar/Debian Board Support Package
MIT License
131 stars 77 forks source link

iot2050-firmware-update: Refactor, bugfix and add some arguments #435

Closed huaqianli closed 1 year ago

huaqianli commented 1 year ago

Add -q|--quiet and -d|--backup-dir arguments, refactor the exit code and fix some bugs.

BaochengSu commented 1 year ago

Otherwise LGTM.

huaqianli commented 1 year ago

@colpane @BaochengSu I did an update, please help review again, thanks.

jan-kiszka commented 1 year ago

I was looking at https://support.industry.siemens.com/forum/ww/en/posts/update-firmware-v1-3-1-1/299468/?page=0&pageSize=10, noticing at least 3 improvables:

Are these issues already in scope for the merge request here?

huaqianli commented 1 year ago
  • no clear reason given why the update is not applicable (most critical issue)
  • "Upgrade Failed! Please do not reboot!!" - as no update was applied, apparently, this warning is misleading
  • We should check compatibility first and, if given, then ask for the ok to apply the update, not the other way around like now

@jan-kiszka There is some improvement here, some of them still act as before.

BaochengSu commented 1 year ago
  • no clear reason given why the update is not applicable (most critical issue)
  • "Upgrade Failed! Please do not reboot!!" - as no update was applied, apparently, this warning is misleading
  • We should check compatibility first and, if given, then ask for the ok to apply the update, not the other way around like now

@jan-kiszka There is some improvement here, some of them still act as before.

Why not fix them in this MR as well? In general, no big change required.

huaqianli commented 1 year ago
  • no clear reason given why the update is not applicable (most critical issue)
  • "Upgrade Failed! Please do not reboot!!" - as no update was applied, apparently, this warning is misleading
  • We should check compatibility first and, if given, then ask for the ok to apply the update, not the other way around like now

@jan-kiszka There is some improvement here, some of them still act as before.

Why not fix them in this MR as well? In general, no big change required.

From my perspective, it is not a small effort especially go with the current PR, If we try to fix all issues in this PR, it will be potentially unstopped. Actually, there is already many unexpected changes to be added in this PR.

If we do want to improve it to the next level, I'd prefer a new PR to do that.

huaqianli commented 1 year ago

A record here: As discussed offline, Colpan was okay with the improvement based on his comment, he mainly cares about how the tool behavior when an error or exception occurs. Especially the return value should be as expected.

Let's go with an example: The tool will return FLASHING_ERR(5) if it fails to upgrade and rollback successfully.

And Colpan, Baocheng and I are okay to do not finish all improvements here, because it will make this PR potentially unstoppable. Maybe a new PR for improving will be issued later.