hagaygo / OpenWrtManager

Mobile app for interacting with your OpenWrt device.
GNU General Public License v3.0
145 stars 10 forks source link

[feature] add ability to reset device #19

Closed mips171 closed 2 years ago

mips171 commented 2 years ago

This also slightly reworks the UI to flow more naturally given this change, by accessing the the DeviceForm via an Edit button, and the commands form via the row item.

Screen Shot 2021-11-06 at 5 24 58 pm Screen Shot 2021-11-06 at 4 58 18 pm Screen Shot 2021-11-06 at 4 58 11 pm
hagaygo commented 2 years ago

Hi,

Thank you for your pull request , I have few questions/issues.

  1. I am not so sure the device reset should be done via this app from various reasons. First of all , device IP might change , even if you use the default IP it still wont be much of help cause username/password is changed and most of the settings are gone and you can not really re-setup the device using this app only , so you'll need a full web interface access to re-setup the device anyhow. Also , its a very "destructive" action , it really should have more warning/steps than a simple yes/no dialog.

  2. I saw you pass 2 parameters to firstboot command , i looked for the command wiki on https://openwrt.org/docs/techref/preinit_mount?s[]=firstboot I am not sure this is the correct way to reset the device, do you have other documentation ? Also , some devices (like my RPI router which uses ext4 file system) does not support factory reset and does not have this option on LuCi so the app should preferably not show this option at all for those setups.

  3. Coding issues You should split the commits as often as possible. You titled the pull request "add ability to reset device" but also add other unrelated changes like adding default ip to be "192.168.1.1" which is not really needded in my opinion (users with OpenWRT setup should know their ip address). also commented on specific code issues.

Thanks again for your efforts.

mips171 commented 2 years ago

Hi,

Thank you for your pull request , I have few questions/issues.

  1. I am not so sure the device reset should be done via this app from various reasons. First of all , device IP might change , even if you use the default IP it still wont be much of help cause username/password is changed and most of the settings are gone and you can not really re-setup the device using this app only , so you'll need a full web interface access to re-setup the device anyhow. Also , its a very "destructive" action , it really should have more warning/steps than a simple yes/no dialog.

I was thinking for a start the dialog buttons could be styled differently, so instead of TextButtons they become RaisedButtons with different colours.

  1. I saw you pass 2 parameters to firstboot command , i looked for the command wiki on https://openwrt.org/docs/techref/preinit_mount?s[]=firstboot I am not sure this is the correct way to reset the device, do you have other documentation ?

I just used the way you suggested and captured the request when issuing reset, and it passed -r for reboot and -y for yes. It does reset the device.

Also , some devices (like my RPI router which uses ext4 file system) does not support factory reset and does not have this option on LuCi so the app should preferably not show this option at all for those setups.

So the question is how would we check whether a device supports fistboot?

  1. Coding issues You should split the commits as often as possible. You titled the pull request "add ability to reset device" but also add other unrelated changes like adding default ip to be "192.168.1.1" which is not really needded in my opinion (users with OpenWRT setup should know their ip address). also commented on specific code issues.

Thanks again for your efforts.

I still have a lot to learn, but thank you for putting this project out for study. If it's not really a necessary feature then it doesn't need to be there.

hagaygo commented 2 years ago

As it stands i don't think a "Reset" functionality should be available in the app.

Off course , feel free to make your own version of the app or ask further questions.

If you want to contribute a features that will probably go into the official app, i suggest trying https://github.com/hagaygo/OpenWRTManager/issues/2 or https://github.com/hagaygo/OpenWRTManager/issues/3