yo1dog/code-prep #52

yo1dog · 2024-10-28T15:26:45Z

Does not change any functionality. Cleans up the code base in preparation for upcoming changes.

Code that is or will be called from multiple locations has been abstracted into functions.
Some code is organized into separate files as main.c was growing quite large.
Global state was removed (and function interfaces updated accordingly).
Some complex function interfaces were simplified using structs.
De-linted to conform with established coding style.

In our previous convo we discussed the fact that their was actually not yet any "established coding style." I left the commits as-is to avoid conflicts in child commits for now. I am fine with whatever style you want of course.

yo1dog · 2024-10-28T15:30:36Z

Just noticed #51

Can wait until that gets merged and then I can rebase.

9ary · 2024-10-28T19:47:55Z

Yeah, let's merge that.

You should be able to follow this to do your rebase automatically: https://stackoverflow.com/a/71812873.

.gitignore

9ary

First round, reviewed up to 90e797b (use BOOT_PAYLOAD struct).

I'll let you rebase and address my comments before continuing.

.gitignore

9ary · 2024-10-28T20:05:45Z

source/main.c

@@ -38,6 +37,18 @@ struct shortcut {
 };
 int num_shortcuts = sizeof(shortcuts)/sizeof(shortcuts[0]);

+u16 all_buttons_held;
+void scan_all_buttons_held()


I realize this is named after the function it calls, but this name feels a bit awkward.

9ary · 2024-10-28T20:08:39Z

source/shortcut.h

+#define INC_SHORTCUT_H
+#include <gctypes.h>
+
+#define NUM_SHORTCUTS 6


You're changing this from calculating the length to hardcoding a value. This is easy to miss and forget updating. Just export the symbol from the new file.

9ary · 2024-10-28T20:16:05Z

source/main.c

      if (all_buttons_held & shortcuts[i].pad_buttons) {
        paths[num_paths++] = shortcuts[i].path;
        break;
      }
    }

-    paths[num_paths++] = default_path;
+    paths[num_paths++] = shortcuts[0].path;


It would be nice if the special case could be removed entirely.
Move the default path to the bottom of the list, then adjust the condition in the loop.

9ary · 2024-10-28T20:19:04Z

source/main.c

+typedef struct
+{
+    u8 *dol;
+    int dol_argc;
+    char *dol_argv[MAX_NUM_ARGV];
+} BOOT_PAYLOAD;


Preferably avoid typedef (and when you must, suffix the name with _t). Type names should be lowercase.

9ary · 2024-10-28T20:24:36Z

source/main.c

@@ -34,7 +38,7 @@ void scan_all_buttons_held()
    );
 }

-void dol_alloc(int size)
+void dol_alloc(u8 **_dol, int size)


We're a two-star codebase now. :)

There is absolutely no reason to use indirection here, simply return the address.

9ary · 2024-10-28T20:26:21Z

source/main.c

 }

-void load_parse_cli(char *path)
+void load_parse_cli(char **_dol_argv, int *_dol_argc, char *path)


Instead of the first two arguments here, take in a pointer to a struct boot_payload.

9ary · 2024-10-28T20:28:02Z

source/main.c

+    BOOT_PAYLOAD payload;
+    payload.dol = NULL;
+    payload.dol_argc = 0;


struct boot_payload payload = { 0 };

- Condense 2 instances of scanning and assigning `all_buttons_held` into a single function.

- Move shortcut definitions to its own file.

- Add a special shortcut definition for default `ipl.dol`. - This prepares for future in which shortcuts are referenced rather than just paths and prevents requiring explicit handling of the special no-shortcut/default case.

- Defines the `BOOT_PAYLOAD` struct for containerizing boot data (`dol` and `dol_argc`). - Removes boot data from global scope.

- Splits `load_parse_cli` into `read_cli_file` and `parse_cli_file`. - Moves DOL file reading to `read_dol_file`.

- Reference shortcuts rather just the shortcut's paths. - Prepares for future in which shortcuts define more than just a path.

- Moves finding and reading of shortcut DOL and CLI files to `load_shortcut_files`.

- Moves all filesystem reading and handling code to its own `filesystem.c` file. - `FS_RESULT` enum extends and replaces `FRESULT` enum. - Colapses `utils.c` into `filesystem.c`. - `dol_alloc` function removed. Filesystem and USB geko routines handle malloc independently. - Dropped `_size` param from `read_cli_file` in favor of `strenlen`

- Rather than track `argc` and `argv` array and combine into `__argv` struct later, create `__argv` struct immediately. - Moves CLI parsing to its own file. - Renames `parse_cli_file` to `parse_cli_args` as it is source agnostic.

- Silence erroneous `TB_BUS_CLOCK` undefined error in some IDEs. - Group global state to top of `main.c`. - Copy and modify rather than overwriting DOL path for CLI path in `read_cli_file`. - `dol` -> `dol_file`, `cli` -> `cli_file`, and other variable name clarifications. - Add comments to describe function return codes. - Add various comments. - Various stylistic changes and delinting for consistency.

yo1dog · 2024-11-05T23:24:14Z

Yeah, let's merge that.

You should be able to follow this to do your rebase automatically: https://stackoverflow.com/a/71812873.

Rebased. Automatic rebase was cool, but the always-take-theirs strategy did the wrong things a few times that I noticed. I didn't really trust it (always skeptical of anything that automates code edits) so I just did the rebase manually.

yo1dog · 2024-11-05T23:43:51Z

A few things to keep in mind:

It's been almost 2 decades since I have done any serious programming in C/C++.
When I originally made these changes, it was just a fun project I was messing with and didn't think they would get merged. So don't read too much into anything that feels opinionated. It's probably not. I don't have strong opinions and you won't hurt my feelings.
These changes were originally authored almost 2 years ago. There's probably some stuff that doesn't make sense anymore.
These were initially a few giant commits that I broke apart after the fact. The result is kind of weird. There may be changes that don't make sense on their own but are utilized later. Or don't mater because they are completely replaced later anyway.
Most of my carrier has been in small, fast team dynamics. So my depth of knowledge on the collaborative side of github is shallow. Forgive me if I clobber pull requests or whatever.

yo1dog · 2024-11-06T00:41:05Z

I went to address some of the comments you have made already, but most seem moot as the change is overwritten or removed in a later commit. Rather than stepping through each commit it may be better to evaluate the final result.

9ary · 2024-11-06T15:04:53Z

A few things to keep in mind:
[...]

All of that's fine. It's a hobby for me too, and there is no rush to get any of this merged. Do let me know if you feel like I'm putting too much pressure on you though, I can be overly perfectionist but I don't mind putting in some work to help get this where I want it.
Overall the improvements look good though, so we're going in the right direction.

These changes were originally authored almost 2 years ago. There's probably some stuff that doesn't make sense anymore.

The code has not actually changed much since then. Most of the work has gone into the new build system, which helped remove almost all prebuilt binaries from the repo (all that's left now is the flasher for the Qoob series, which will be replaced by open source rewrites in due time).

Rather than stepping through each commit it may be better to evaluate the final result.

It's important that the individual commits make sense. For one, it's much easier to review commits one by one than the final diff, and it's also useful in the future to understand why the code looks the way it does.

but most seem moot as the change is overwritten or removed in a later commit.

This possibly warrants a reorganization of the commits then? Do let me know what doesn't make sense to you and we can try to adjust that.

yo1dog · 2024-11-07T23:51:37Z

This possibly warrants a reorganization of the commits then? Do let me know what doesn't make sense to you and we can try to adjust that.

Eh. Probably not. When I did the work of splitting up the commits it was for that purpose: So that the changes could be stepped through to get a clear view of the history. But I wasn't necessarily trying to get every commit to stand on its own, which sounds like is your goal. To that end I am happy to make inter-commit fixes to clean it up, even if it doesn't affect the final result because it is replaced in a later commit.

Funny enough, I have spent way more time rebasing and splitting up commits than I did on the actual improvements lol.

9ary · 2024-11-09T22:01:43Z

But I wasn't necessarily trying to get every commit to stand on its own, which sounds like is your goal.

Here's the thing: if the commits are split for reviewability, they should be reviewed as such. In general, it's a common expectation that every commit will at least build and run (and pass tests, but we don't have those). This helps understand the context for a change, and it makes tool like git bisect actually useful.

As a tangential note, I've been putting a lot of work into making gekkoboot reproducible. This means that the repo contains enough information at this point for anyone to clone it at any point in the future, and build any commit from source with a matching checksum. This is extremely valuable for debugging, and it's also why I care so much about history that makes sense.

even if it doesn't affect the final result because it is replaced in a later commit.

Can you point out such an occurrence within this PR? I agree that it's not really worth it to put too much effort into transitional states, but I'm also a bit confused about what you mean. What changes are going to be replaced later that I commented on? I'm happy to accept some imperfect things if they're not going to stay, just show me what they are.

Funny enough, I have spent way more time rebasing and splitting up commits than I did on the actual improvements lol.

Hopefully we're done with major rebases. Sorry about the whole formatting thing, I'm putting the master branch on a soft freeze until we're done (or at least have made significant progress) with merging your work.

9ary reviewed Oct 28, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

9ary requested changes Oct 28, 2024

View reviewed changes

yo1dog added 10 commits November 5, 2024 17:16

add scan_all_buttons_held function

7512aaa

- Condense 2 instances of scanning and assigning `all_buttons_held` into a single function.

move shortcuts to file

7790c74

- Move shortcut definitions to its own file.

use shortcut for default

fc572ad

- Add a special shortcut definition for default `ipl.dol`. - This prepares for future in which shortcuts are referenced rather than just paths and prevents requiring explicit handling of the special no-shortcut/default case.

use BOOT_PAYLOAD struct

1634ea8

- Defines the `BOOT_PAYLOAD` struct for containerizing boot data (`dol` and `dol_argc`). - Removes boot data from global scope.

split reading and parsing of files

501009e

- Splits `load_parse_cli` into `read_cli_file` and `parse_cli_file`. - Moves DOL file reading to `read_dol_file`.

pass shortcut index instead of paths array

3e37461

- Reference shortcuts rather just the shortcut's paths. - Prepares for future in which shortcuts define more than just a path.

split load_shortcut_files function

d990442

- Moves finding and reading of shortcut DOL and CLI files to `load_shortcut_files`.

pass argv struct instead of arg array

91e86cc

- Rather than track `argc` and `argv` array and combine into `__argv` struct later, create `__argv` struct immediately. - Moves CLI parsing to its own file. - Renames `parse_cli_file` to `parse_cli_args` as it is source agnostic.

yo1dog force-pushed the yo1dog/code-prep branch from 9c25698 to 87fd89a Compare November 5, 2024 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

yo1dog/code-prep #52

yo1dog/code-prep #52

yo1dog commented Oct 28, 2024

yo1dog commented Oct 28, 2024

9ary commented Oct 28, 2024

9ary left a comment

9ary Oct 28, 2024

9ary Oct 28, 2024

9ary Oct 28, 2024

9ary Oct 28, 2024

9ary Oct 28, 2024

9ary Oct 28, 2024

9ary Oct 28, 2024

yo1dog commented Nov 5, 2024

yo1dog commented Nov 5, 2024

yo1dog commented Nov 6, 2024

9ary commented Nov 6, 2024

yo1dog commented Nov 7, 2024

9ary commented Nov 9, 2024

yo1dog/code-prep #52

Are you sure you want to change the base?

yo1dog/code-prep #52

Conversation

yo1dog commented Oct 28, 2024

yo1dog commented Oct 28, 2024

9ary commented Oct 28, 2024

9ary left a comment

Choose a reason for hiding this comment

9ary Oct 28, 2024

Choose a reason for hiding this comment

9ary Oct 28, 2024

Choose a reason for hiding this comment

9ary Oct 28, 2024

Choose a reason for hiding this comment

9ary Oct 28, 2024

Choose a reason for hiding this comment

9ary Oct 28, 2024

Choose a reason for hiding this comment

9ary Oct 28, 2024

Choose a reason for hiding this comment

9ary Oct 28, 2024

Choose a reason for hiding this comment

yo1dog commented Nov 5, 2024

yo1dog commented Nov 5, 2024

yo1dog commented Nov 6, 2024

9ary commented Nov 6, 2024

yo1dog commented Nov 7, 2024

9ary commented Nov 9, 2024