portapack-mayhem / mayhem-firmware

Custom firmware for the HackRF+PortaPack H1/H2
GNU General Public License v3.0
3.22k stars 530 forks source link

sdcard structure refactor - firmware part #1967

Closed zxkmm closed 2 months ago

zxkmm commented 5 months ago

for easier review, i separated firmware change part and sdcard change part. This is firmware change part please don't merge as i need to merge myself. test build(put world map yourself): https://github.com/zxkmm/mayhem-firmware/releases/tag/test_release

NotherNgineer commented 5 months ago

I noticed that the text still needs to be updated in several of the display_modal messages to show the expected folder name. For example: ui_geomap.cpp and ui_sstvtx.cpp (maybe others).

zxkmm commented 5 months ago

Ohhhhh right, thank you so much for the remind!

zxkmm commented 5 months ago

Will do all the requests changes after lunch. Thx!

zxkmm commented 5 months ago

fixed all the review

NotherNgineer commented 5 months ago

This PR is getting very close. To make it perfect, I think we should provide an easy one-click YES/NO modal that automates movement of pre-existing user files and directories to the new folder locations when we detect the old folder layout. The user should not have to connect the PortaPack or SD Card to a computer and manually move their existing folders based on some instructions in the Wiki. To automate the SDcard upgrade process, we should create a table of directories to look for, with a flag indicating whether they should be copied to /USR or /RES. Hopefully it won't be too hard to move a directory in firmware. (Of course there are certain folders that currently contain a mix or user and system files like FREQMAN, so we'd probably just pick the USR folder in that case.)

zxkmm commented 5 months ago

This PR is getting very close. To make it perfect, I think we should provide an easy one-click YES/NO modal that automates movement of pre-existing user files and directories to the new folder locations when we detect the old folder layout. The user should not have to connect the PortaPack or SD Card to a computer and manually move their existing folders based on some instructions in the Wiki. To automate the SDcard upgrade process, we should create a table of directories to look for, with a flag indicating whether they should be copied to /USR or /RES. Hopefully it won't be too hard to move a directory in firmware. (Of course there are certain folders that currently contain a mix or user and system files like FREQMAN, so we'd probably just pick the USR folder in that case.)

Sure, however, which dir to move? I think we can only do CAPTURES

zxkmm commented 5 months ago

And another point is that i'm not sure if it's safe to move that many files in PP. since from what i observed, fatfs isn't that stable.

zxkmm commented 5 months ago

And still another point is, since they still need to move or merge others file on computer, why do a half of works on portapack anyway? and it also bring many redundant codes which could only been used as once.

NotherNgineer commented 5 months ago

Can I also ask why you changed the name of the system/resource folder from "SYS" to "RES"? Seems like the preference was "SYS" in most of the options in #1980.

@zxkmm : Sorry that to make it more clear, I just answering by editing this message:It’s because I think it’s more accurate to call it resources than system.

NotherNgineer commented 5 months ago

And still another point is, since they still need to move or merge others file on computer, why do a half of works on portapack anyway? and it also bring many redundant codes which could only been used as once.

Let me try to explain my reasoning:

The WHOLE POINT of this PR is that it's supposed to make it easier for the user to upgrade. If we tell the user that they need to study the Wiki and spend an hour copying their files (remotes, play lists, log files, splash screens, captures, etc) to the new folders after flashing this firmware, then we have NOT made the process easier for them.

SD card upgrades should be easier in the FUTURE after this PR, but I think we need to make the process easy for them on THIS upgrade too.

We should know where to copy most of the files, since we'll have to explain it in said Wiki.

Yes there is a concern regarding how to handle directories that may contain a mix of user and system files, in which case my thought was to copy them to the /USR folder to make sure they don't get deleted if the user indeed deletes the whole /RES (or /SYS) folder.

zxkmm commented 5 months ago

The WHOLE POINT of this PR is that it's supposed to make it easier for the user to upgrade.

Yes.

If we tell the user that they need to study the Wiki and spend an hour copying their files (remotes, play lists, log files, splash screens, captures, etc) to the new folders after flashing this firmware, then we have NOT made the process easier for them.

I’m sorry but this is not true. First of all, they basically just need to select several folders and move to a sub-folders. Everyone who know basic computer knowledge can do this within 5 minutes. And please note that before my change, users need to do this during every update, I already made it way simpler.

SD card upgrades should be easier in the FUTURE after this PR, but I think we need to make the process easy for them on THIS upgrade too. We should know where to copy most of the files, since we'll have to explain it in said Wiki. Yes there is a concern regarding how to handle directories that may contain a mix of user and system files, in which case my thought was to copy them to the /USR folder to make sure they don't get deleted if the user indeed deletes the whole /RES (or /SYS) folder.

I apologize, I would never implement auto move files feature. It’s too dangerous.

Despite the fact that it could be dangerous to handle bunch of big files with an old version of fatfs, what if users sdcard have dead block where the new fat index are broken, leading to file lost? What if the battery is already low, and powered off during moving, leading to file damage? Like I said, asking users to move several files on computer is not as scary as you said, it’s literally just move some files on a computer which they do it almost all the time.

I’m so sorry about this chat. I have no idea why you hate my design so much. I don’t know if it’s personal or something else.

Sorry that I know that you are a better and more experienced developer than me. And I know that you got more call than me in mayhem.

So I’m sorry to pointed it out, if you hate this design, or hate me, I’m totally ok if you want to close this PR. I can leave forever. That’s all.

I’m sorry that I have to go to this step. I respect you and I respect your work. I know you hate me or my work, it’s probably time for me to show myself out. Thanks.

zxkmm commented 5 months ago

However if this could make you feel better: I can support the old file’s structure, but show a modal msg in every boot, until they correctly put files.

NotherNgineer commented 5 months ago

@zxkmm Please don't take my review comments personally. I have nothing against you, and I appreciate that you have done some great work on Mayhem, and you have been contributing to it for a lot longer than I have. (I am also not trying to drive you crazy by proposing additional suggestions over time.)

It is simply the user experience that I am questioning. Changing the directory structure is a huge change which causes both forward and backward FW compatibility issues. If we change the directory structure, I would just like to make sure that the next firmware upgrade is as seamless as possible for users. Even if we assume that the user downloads the new SD card image and installs it properly, there's still the issue that their existing user files need to be moved. I know that you don't like the idea, but I feel that we can automate most of that file movement after an upgrade.

I have also come up with a thought on how we could separate the "user" files from "system" files in folders which may contain a mix of both: I think we could look at the file time stamps. If the files in the FREQMAN folder all have the same time stamp as the FREQMAN folder itself, then we could "assume" that they're "system" files. Files with a newer time stamp we could assume are "user" files.

If another developer feels that the automated file movement is not necessary and would like to go ahead and approve this PR without it, that would be great. There are many other Mayhem developers, so don't leave this decision solely up to me. In that case, I would probably write code to do the file movement anyway and propose it in a follow-on PR.

If for some reason we don't end up moving the directories around as the PR proposes, I think we could still make upgrades easier for the user by creating a large .tar upgrade file that includes the full SD card image as I've proposed in enhancement #1996.

zxkmm commented 5 months ago

Thank you for your clarification. How about this: I implement support for both file structure, so if user somehow decided not to change the sdcard structure, they can still use it, only need to see the modal once. then they will only have two more useless folder (RES and USR) in their root dir.

But I need to add a uint 8 to pmem. To do this way.

NotherNgineer commented 5 months ago

Thank you for your clarification. How about this: I implement support for both file structure, so if user somehow decided not to change the sdcard structure, they can still use it, only need to see the modal once. then they will only have two more useless folder (RES and USR) in their root dir.

But I need to add a uint 8 to pmem. To do this way.

Having firmware support both the old and new folder structure sounds like a good idea.

Referring to another comment above, I wanted to note that it in "SD over USB" mode it takes about 3 hours to "copy" the world map file from the old folder to a new folder, whereas we could do it in firmware in a few seconds. The Wiki will have to tell them to remove the SD card I suppose.

zxkmm commented 5 months ago

Having firmware support both the old and new folder structure sounds like a good idea.

Thank you very much for accepting this suggestion. I can implement it if you believe it's necessary.

Lemme be clear: After this feat implemented, will you provide some positive comments of this design, or will you still hate/against it? I need an answer, because I need it for my next step move; Since I'm pretty aware that no one will approve or even agree to this change if you still don't like it. So if you don't like it, feel free to tell me, then I won't do it, and I'll wait for BDFL's to close this PR.

NotherNgineer commented 5 months ago

Having firmware support both the old and new folder structure sounds like a good idea.

Thank you very much for accepting this suggestion. I can implement it if you believe it's necessary.

Lemme be clear: After this feat implemented, will you provide some positive comments of this design, or will you still hate/against it? I need an answer, because I need it for my next step move; Since I'm pretty aware that no one will approve or even agree to this change if you still don't like it. So if you don't like it, feel free to tell me, then I won't do it, and I'll wait for BDFL's to close this PR.

I cannot guarantee that I'll like your implementation until I see it. Most likely I will still question whether this PR is useful or just causing needless incompatibilities between firmware versions.

zxkmm commented 5 months ago

@NotherNgineer i see. Thank you for the clarification. Then would you mind if I leave the change requests until the BDFL decides whether we want it? Because it would be sad that I do more on it, but closed at the end. And take your time, we may can wait and see. Which means I don’t want you to waste your time reviewing it, like I always say. (It’s not sarcasm, just literal meant)

eried commented 5 months ago

BDFL

Lol

zxkmm commented 5 months ago

BDFL

Lol

It's a common case in FOSS. Since Nother clearly declared that he doesn't like this change, So I need to know what's the next step before I do more on it. Since it would become useless if it will close in the end.

NotherNgineer commented 5 months ago

I still believe that this change will be too disruptive for most users and I predict lots of confusion and likely negative feedback on social media.

In any case, could you post a new test build in the Discord #test-drive channel with all of the latest changes?

zxkmm commented 5 months ago

@jLynx @NotherNgineer Thank you so much for the review! I will fix them and then directly resolve conversations. If there's any disagree/ issues, i will directly reply in conversations. But if didn't reply, it means resolved. Thx again!

NotherNgineer commented 5 months ago

@zxkmm, thank you for taking the time to address my major concerns.

Although I still don't see much user benefit (and question how much code space is consumed), I think these PR's are a lot better now. Because this is a major change, I think we should wait until after a 2.0.1 release before deciding what to do with them.

eried commented 2 months ago

I'm closing this PR for now due to a lack of consensus on its benefits. We can revisit and reopen it if compelling reasons for the change come up.