Hi Sam,
On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote:
Hi Eugeniu,
On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca erosca@de.adit-jv.com wrote:
[..]
Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it?
Documentation would be nice. Although you already provided a generic description of 'bcb' command in 'help bcb', the user often wants to read more high-level documentation. I'd say, you can copy the description from commit message to doc/README.android-bcb, extending it with usual use-cases, and how this command can be used in those use-cases. For example, your pseudo-code for reboot reason you provided to me earlier, etc.
Agreed. In my queue.
Also, it can be useful to mention if Google have any requirements (mandatory or optional) for the bootloader (misc partition, reboot reason, recovery, etc), and how this 'bcb' command can help in those requirements implementation.
Well, a subset of the Android bootloader requirements is publicly available via https://source.android.com/devices/bootloader, but I saw traces of the "Android Boot/Bootloader Requirements" document name mentioned in the descriptions of U-Boot commits received from our suppliers. I _do not_ have access to the latter. If anybody from Google sees this message and can share the document or a subset of it, that would be great.
To be clear, the background behind the 'bcb' command is not because I was assigned the task to implement any of the Android bootloader requirements, but because I saw improper implementations of some of those requirements in the patches received from our supplier and wanted to showcase a better/U-Boot-compliant way to accomplish those.
So, I don't give any commitment to support the Android-related parts in U-Boot, but will signal and express my opinion whenever any features backported from AOSP don't follow the patterns established in community.
All that said, IMHO documentation/test wise: it's not critical in this case, you can add that later, just to speed-up the whole development process and divide it into iterations. But that's for maintainers to decide, of course.
Also, I've a feeling that we have another problem, more common than just a documentation. At the moment we have a bunch of Android related features, which don't have namespace separation on several levels:
- file/directories: we may want to move all Android related commands
to sub-directory
- commands: we may want to add main command called "android" for all
Android-related commands, or maybe just a prefix
- Kconfig: we may want to have some generic naming scheme for all
Android-related commands
- Documentation, tests: the same here
So at some point we will probably need to discuss and fix that somehow. Not here, of course :)
Agree with the most of the bullets. However, WRT merging all the available Android commands into a single command, I see room for more discussion. Here is some valuable feedback from Ruslan Trofymenko:
--- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 --- On Thu, 6 Dec 2018 at 03:31, Simon Glass sjg@chromium.org wrote:
Perhaps we need a new 'android' command with an 'ab_select' subcommand? Then the automatica abbreviation will work.
Agree with all your previous comments, will send v2 shortly. But this one I'd like to leave as is (I will drop android_ prefix though). I think adding "android" command may lead to unneeded architecture complexity in future, e.g.: - we will need to re-work next commands as sub-commands of "android" command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select command file has BSD license and other commands have GPL license) - we will need to implement sub-sub-commands (e.g. for "android dtimg dump ..." etc.) - having "android" command can be inconvenient for users and may break existing API (e.g. it will force users to use "android fastboot" instead of just "fastboot" command) - actually I don't see any namespace issues that can be solved by adding "android" command
Also, in subsequent patch, I will add the dedicated menu option for Android-related commands and will pull all existing Android commands (along with ab_select) to that menu entry.
So I hope it's fine with you if I leave this command as "ab_select"? --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 ---
In addition, based on the feedback from Alex Kiernan, 'bcb' might be useful in non-Android contexts. If so, then embedding it as sub-command into a higher level command (e.g. 'android') does not make much sense.