cmd: migrate from getopt to argp #73

sinuscosinustan · 2024-12-09T10:20:04Z

Reopened because of fail in #71 / #70

The argument handling is quite hard to make it good when using getopt with help and usage messages. The argp library is a better choice for this task.

Note: cleanup of dirty code or unwanted redundancies will be done from time to time in this PR. If you notice anything that bothers you, please add a comment directly. Thanks!

Checklist:

Migrate command base structure to argp
Refactor host_init and ast_ahb_access for standardised flags
Cleanup inconsistencies
Consolidate possible duplicates and make it more user-friendly
Add proper git commit messages
Documentation

sinuscosinustan · 2024-12-10T16:37:58Z

@amboar I don't have an AST2600 system on-hand to test the coprocessor stuff. Could you please test if the command still works as before?

amboar · 2024-12-11T00:22:24Z

@amboar I don't have an AST2600 system on-hand to test the coprocessor stuff. Could you please test if the command still works as before?

Looks like the argument processing is a bit off:

root@ast2600:~# ./culvert -vv coprocessor run 0xba000000 $((32 * 1024 * 1024)) < sclydesdale.bin
[*] Found 5 registered bridge drivers
[*] Trying bridge driver p2a
[*] Trying bridge driver ilpc
[*] Trying bridge driver devmem
[*] Trying bridge driver debug-uart
[*] Opening 33554432
[*] Failed to initialise the console (33554432): -1
[*] Failed to initialise local debug interace on 33554432: -1
[*] Trying bridge driver l2a
[*] Bridge discovery failed, cannot access BMC AHB
[*] Failed to acquire AHB interface

It should instead look like:

root@ast2600:~# ./culvert -vv coprocessor run 0xba000000 $((32 * 1024 * 1024)) < sclydesdale.bin
[*] Found 5 registered bridge drivers
[*] Trying bridge driver p2a
[*] Failed to initialise P2A bridge: -2
[*] Trying bridge driver ilpc
[*] Failed to initialise iLPC bridge: -95
[*] Trying bridge driver devmem
[*] Probing devmem
[*] Probing for SoC revision registers
[*] ahb_readl: 0x1e6e2004: 0x05030303
[*] ahb_readl: 0x1e6e207c: 0x00000000
[*] ahb_readl: 0x1e6e2014: 0x05030303
[*] Found revision 0x5030303
[*] Trying bridge driver debug-uart                                            
[*] Unrecognised argument list for debug interface (0)    
...

sinuscosinustan · 2024-12-11T12:01:04Z

Looks like the argument processing is a bit off:

Found the issue. That was a leftover when I made the subcommand stuff easier 😄

New commit pushed where the argument processing for host_init is now correct.

amboar · 2024-12-12T04:02:59Z

Currently:

root@ast2600:~# ./culvert -vv coprocessor run 0xba000000 $((32 * 1024 * 1024)) < sclydesdale.bin
[*] Found 5 registered bridge drivers
[*] Trying bridge driver p2a
[*] Trying bridge driver ilpc
[*] Trying bridge driver devmem
[*] Trying bridge driver debug-uart
[*] Unrecognised argument list for debug interface (2)
[*] Trying bridge driver l2a
[*] Bridge discovery failed, cannot access BMC AHB
[*] Failed to acquire AHB interface

🙂

I suspect there's a stray two arguments being passed to devmem_driver_probe(), which then takes an early-exit.

sinuscosinustan · 2024-12-12T14:30:02Z

I suspect there's a stray two arguments being passed to devmem_driver_probe(), which then takes an early-exit.

I'll migrate all commands and then implement a generic struct for the access stuff that's being used in like every command.

That should make it easier I'd say instead of fiddling with argc and argv shit again like it has been done before.

The argument handling is quite hard to make it good when using `getopt` with help and usage messages. The `argp` library is a better choice for this task. As there are several commands which have subcommands, a new helper function called `parse_subcommand` has been added to simplify the migration to `argp`. It'll take care of moving the subcommand to `argv[0]` and updating `argc` and `argv` accordingly, but also do `argp_parse`. Signed-off-by: Tan Siewert <[email protected]>

This is the first command that's being reworked for argp. Signed-off-by: Tan Siewert <[email protected]>

Signed-off-by: Tan Siewert <[email protected]>

This subcommand differs from the others a bit in regards to sub-subcommands. `read` and `write` are handled in `parse_opt` directly instead of creating two new subcommands, because the actual handling for reading and writing is done in ast_ahb_access, where it checks if argv[0] is the operation to be performed. Signed-off-by: Tan Siewert <[email protected]>

The Debug UART inside the BMC can be brought into a stuck state by sending a lot of requests quickly or after sending a broken command. Sending 2 exit commands at 115200 baud before trying to enter debug mode again seems to mitigate this issue. Co-authored-by: Tan Siewert <[email protected]> Signed-off-by: Sarah Maedel <[email protected]> Signed-off-by: Tan Siewert <[email protected]>

The debug command now has two separate methods for read and write. While the functionality is almost the same, the code is now more readable and maintainable. Signed-off-by: Tan Siewert <[email protected]>

Similar to the devmem command, the determination of read/write is done in parse_opt, mainly because ast_ahb_access is doing shenanigans. Signed-off-by: Tan Siewert <[email protected]>

The JTAG argument handling is very special compared to the other commands. This command spawns a network server for remote bitbang. There's definitely a bit rework needed because the new implementation does wild things for host_init. Signed-off-by: Tan Siewert <[email protected]>

Commit 7db29d2 added the devcontainer manifests that are used in IDEs like Visual Studio Code. Unfortunately, VSC creates user configs in the workspace (project in this case). Subprojects are being pulled by meson in the `subprojects` directory and should not be checked in Git. Signed-off-by: Tan Siewert <[email protected]>

Signed-off-by: Tan Siewert <[email protected]>

sinuscosinustan · 2024-12-12T21:30:42Z

All commands now use argp, but they are still somewhat inconsistent, with parsing handled differently in some cases.

I will work on resolving these inconsistencies and aim to make the implementation more generic.

For readers and contributors, I’d appreciate your feedback on the documentation for the commands! Some commands are based on guesses regarding their functionality, so it would be helpful to ensure the documentation is accurate.

amboar · 2024-12-13T04:44:10Z

@sinuscosinustan something I have been idly considering for a while is to require a keyword like via (in the vein of ip route ...) to help sync the argument parsing where the user wants a specific interface to be used, e.g:

# culvert write firmware via /dev/ttyUSB0

or maybe even with the bridge name:

# culvert write firmware via debug /dev/ttyUSB0

What do you think?

This might help where there are other positional arguments, such as in the case of the coprocessor run subcommand (which I've just tested again and isn't yet working).

For the record, I plan to tag a release of culvert immediately prior to merging your changes. I think it's reasonable that we can break the command line behaviour a bit subsequent to the tag, in the process of cleaning up the implementation.

Thanks again for your effort here. I'm honestly not sure how you've motivated yourself through it, because I was certainly struggling to motivate myself 😅

sinuscosinustan · 2024-12-13T14:30:24Z

What do you think?

I was thinking about a generic set of options like -I /dev/ttyUSB0 -H <IP> and so on, but using via is also a cool idea!

For the record, I plan to tag a release of culvert immediately prior to merging your changes

Awesome! At least some breakage possible 😄

I'm honestly not sure how you've motivated yourself through it

Holiday, a lot of caffeine and suffering from the broken parsing 😂

Signed-off-by: Tan Siewert <[email protected]>

sinuscosinustan · 2024-12-13T16:13:26Z

I've pushed commit ae1c35c as proposal (I know it'll cause pointer errors and whatsoever in its current state).

@amboar What do you think about this? Would this be suitable or would be the via approach better?

amboar · 2024-12-17T22:49:49Z

I've pushed commit ae1c35c as proposal (I know it'll cause pointer errors and whatsoever in its current state).

@amboar What do you think about this? Would this be suitable or would be the via approach better?

Sorry for the delay; after a quick look I think I prefer the via approach? I'm happy to prototype it myself if you don't prefer that direction (you've done a lot of great work already and I don't want you to feel obligated to any requests on my part).

sinuscosinustan · 2024-12-19T09:55:28Z

after a quick look I think I prefer the via approach?

Sounds good! I've tried to take a look into implementing my approach but for some reason argp doesn't want me to use the children option for that and always segfaults with non-explainable coredumps (lol).

I'm happy to prototype it myself if you don't prefer that direction

If you have a small prototype of how it should look like I'd be happy to implement it! In the meanwhile I'll rework the host_init stuff to use struct connection instead and cleanup any inconsistencies.

sinuscosinustan force-pushed the argument-parsing branch 3 times, most recently from ab2ead5 to 29b192a Compare December 10, 2024 16:23

sinuscosinustan force-pushed the argument-parsing branch 6 times, most recently from 9acb55b to cb6c555 Compare December 10, 2024 19:50

sinuscosinustan mentioned this pull request Dec 10, 2024

bridge: debug: Always try to exit debug first #60

Open

sinuscosinustan force-pushed the argument-parsing branch 3 times, most recently from afd6f36 to 7338760 Compare December 11, 2024 22:02

sinuscosinustan and others added 12 commits December 12, 2024 15:18

cmd: console: rework for argp

daa5897

This is the first command that's being reworked for argp. Signed-off-by: Tan Siewert <[email protected]>

cmd: coprocessor: rework for argp

7111803

Signed-off-by: Tan Siewert <[email protected]>

cmd: debug: rework for argp

690ceda

The debug command now has two separate methods for read and write. While the functionality is almost the same, the code is now more readable and maintainable. Signed-off-by: Tan Siewert <[email protected]>

cmd: ilpc: rework for argp

dde5e2f

Similar to the devmem command, the determination of read/write is done in parse_opt, mainly because ast_ahb_access is doing shenanigans. Signed-off-by: Tan Siewert <[email protected]>

cmd: p2a: rework for argp

e650982

Signed-off-by: Tan Siewert <[email protected]>

cmd: probe: rework for argp

824d413

Signed-off-by: Tan Siewert <[email protected]>

cmd: otp: rework for argp

d3c5214

Signed-off-by: Tan Siewert <[email protected]>

sinuscosinustan added 2 commits December 12, 2024 15:18

cmd: reset: rework for argp

c322d28

Signed-off-by: Tan Siewert <[email protected]>

cmd: trace: rework for argp

916ddbf

Signed-off-by: Tan Siewert <[email protected]>

sinuscosinustan force-pushed the argument-parsing branch from 7338760 to 916ddbf Compare December 12, 2024 15:38

sinuscosinustan added 4 commits December 12, 2024 21:24

cmd: read: rework for argp

8b55455

Signed-off-by: Tan Siewert <[email protected]>

cmd: replace: rework for argp and fix match_len check

b863de6

Signed-off-by: Tan Siewert <[email protected]>

cmd: sfc: rework for argp

ccec091

Signed-off-by: Tan Siewert <[email protected]>

cmd: write: rework for argp

ea17f51

Signed-off-by: Tan Siewert <[email protected]>

sinuscosinustan force-pushed the argument-parsing branch from e08ed15 to ea17f51 Compare December 12, 2024 21:26

(unused) proposal: add standarised connection struct

ae1c35c

Signed-off-by: Tan Siewert <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd: migrate from getopt to argp #73

cmd: migrate from getopt to argp #73

sinuscosinustan commented Dec 9, 2024 •

edited

Loading

sinuscosinustan commented Dec 10, 2024

amboar commented Dec 11, 2024 •

edited

Loading

sinuscosinustan commented Dec 11, 2024

amboar commented Dec 12, 2024

sinuscosinustan commented Dec 12, 2024 •

edited

Loading

sinuscosinustan commented Dec 12, 2024

amboar commented Dec 13, 2024

sinuscosinustan commented Dec 13, 2024 •

edited

Loading

sinuscosinustan commented Dec 13, 2024 •

edited

Loading

amboar commented Dec 17, 2024

sinuscosinustan commented Dec 19, 2024

cmd: migrate from getopt to argp #73

Are you sure you want to change the base?

cmd: migrate from getopt to argp #73

Conversation

sinuscosinustan commented Dec 9, 2024 • edited Loading

sinuscosinustan commented Dec 10, 2024

amboar commented Dec 11, 2024 • edited Loading

sinuscosinustan commented Dec 11, 2024

amboar commented Dec 12, 2024

sinuscosinustan commented Dec 12, 2024 • edited Loading

sinuscosinustan commented Dec 12, 2024

amboar commented Dec 13, 2024

sinuscosinustan commented Dec 13, 2024 • edited Loading

sinuscosinustan commented Dec 13, 2024 • edited Loading

amboar commented Dec 17, 2024

sinuscosinustan commented Dec 19, 2024

sinuscosinustan commented Dec 9, 2024 •

edited

Loading

amboar commented Dec 11, 2024 •

edited

Loading

sinuscosinustan commented Dec 12, 2024 •

edited

Loading

sinuscosinustan commented Dec 13, 2024 •

edited

Loading

sinuscosinustan commented Dec 13, 2024 •

edited

Loading