Great Modules Refactor by solara404 · Pull Request #274 · pmotionf/mmc-cli

solara404 · 2026-03-31T07:20:03Z

solara404 · 2026-03-31T07:54:46Z

@aaumar25
There is still issues that need to be worked on but i think this would give a good overview. I uploaded mmc_client.zig and MmcClient.zig. These files are pretty much the same, but mmc_client.zig allows comparison.

aaumar25 · 2026-04-01T00:38:37Z

Thanks for the attempt. I want to point out the direction a little bit here as well.

~~The init() function in each module will return its own type, for instance in MmcClient.zig:~~

pub fn init(c: Config) !MmcClient{}

In command.zig, we already have variable that track which module is already initialized, 'initialized_modules. The plan is to add modules we already load_configintoinitialized_modules, and create a function to deinitialize module in command.zig` by passing the module.

Right now, in loadConfig() in command.zig, we always deinitialized modules. Lets refactor the behavior to not deinitialize all modules that already initialized before. Instead, if the module that we are going to initialize is already initialized, we deinitialized what is already been initialized first.

~~From here, lets point out and resolve any issue that may appear with this approach.~~

EDIT: Wait, I just realized it is not possible to do that on current structure. However, my intent is to store everything on the command.zig. Lets figure out how to achieve that.

I think, lets still make init() function to return void similar as before

solara404 · 2026-04-01T01:38:41Z

I transferred the history of mmc_client.zig to MmcClient.zig. I will look into the mentioned changes.

solara404 · 2026-04-02T06:40:04Z

There is still the following in the code:

pub fn get() *MmcClient {
    return &(current orelse std.debug.panic("Mmc_client module is not initialized", .{}));
}

It is not ideal, but MmcClient commands are now registered and unregistered in command.zig, so this situation should not occur. The commands which would trigger this would no longer be accessible when the module is deinitialized.

However, I am looking into it to replace this.

aaumar25 · 2026-04-03T00:03:21Z

I only see briefly the changes you made. I saw in MmcClient module it has create and init, while the init is only calling create. Is there a specific reason behind this? As a side note, the errdefer line in the init() of MmcClient is no op.

solara404 · 2026-04-03T01:20:20Z

init() was just made for ModuleSpec. Oh yes the errordefer is pointless here, I will remove that.

aaumar25 · 2026-04-03T02:32:58Z

Then, is there a point to have create() function there?

solara404 · 2026-04-03T04:33:53Z

It is not necessary, the intend was create() creates MmcClient and init() puts it into global current variable.
I will put everything into init() to remove the extra step.

aaumar25 · 2026-04-21T00:06:26Z

Can you give me some way to review it? This PR is massive. It is so hard to review it.

aaumar25 · 2026-04-21T00:19:44Z

Also, try to squash small commits into one commit that still under the same scope, and put some thought on naming that commit title. Put some emphasize to the commit name of what is being changed or what is the impact of that commit.

solara404 · 2026-04-21T00:36:47Z

Okay, I will. Regarding the review, it looks extensive, but the majority of the changes are in command.zig.

aaumar25 · 2026-04-21T07:23:48Z

+/// Module lifecycle hooks to load and unload modules and its module
+/// commands set.
+const ModuleSpec = struct {
+    init: *const fn (Config.ModuleConfig) anyerror!void,
+    deinit: *const fn () void,
+    commands: []const Command,
+};
+
+/// Stub initializer for modules disabled at build time.
+fn initModuleDisabled(_: Config.ModuleConfig) !void {
+    return error.ModuleDisabledAtBuildTime;
+}
+
+/// Stub deinitializer for modules disabled at build time.
+fn deinitModuleDisabled() void {}
+
+fn initMmcClient(module_config: Config.ModuleConfig) !void {
+    const cfg = switch (module_config) {
+        .mmc_client => |cfg| cfg,
+        else => unreachable,
+    };
+    try mmc_client.init(cfg);
+}
+
+fn initMes07(module_config: Config.ModuleConfig) !void {
+    const cfg = switch (module_config) {
+        .mes07 => |cfg| cfg,
+        else => unreachable,
+    };
+    try mes07.init(cfg);
+}
+
+const module_specs = std.EnumArray(Config.Module, ModuleSpec).init(.{
+    .mmc_client = if (config.mmc_client) .{
+        .init = initMmcClient,
+        .deinit = mmc_client.deinit,
+        .commands = mmc_client.module_commands[0..],
+    } else .{
+        .init = initModuleDisabled,
+        .deinit = deinitModuleDisabled,
+        .commands = &.{},
+    },
+    .mes07 = if (config.mes07) .{
+        .init = initMes07,
+        .deinit = mes07.deinit,
+        .commands = mes07.module_commands[0..],
+    } else .{
+        .init = initModuleDisabled,
+        .deinit = deinitModuleDisabled,
+        .commands = &.{},
+    },
+});
+


These lines smells so bad. Whenever adding new modules or removing modules, requires developer to manually remove it.

ㅜㅜㅜ, ok I will look into it.
Wait a little bit I am going to force push soon. I squashed quite a bit.

aaumar25 · 2026-04-21T07:34:05Z

+const LoadedConfig = struct {
+    id: []const u8,
+    source_path: []const u8,
+    arena: *std.heap.ArenaAllocator,


what is this arena doing here?

aaumar25 · 2026-04-21T07:36:26Z

+        self.config.deinit();
+        self.arena.deinit();
+        std.heap.smp_allocator.destroy(self.arena);
+        std.heap.smp_allocator.free(self.id);
+        std.heap.smp_allocator.free(self.source_path);


In zig, whenever a function require an allocator, you should pass the allocator through the function or store the allocator as a field in the struct.

However, zig now moves to never store allocator as a field in the struct as you can see they are removing anything callled xxxManaged from their own standard library.

…th disabled modules

aaumar25

This PR has major problems with allocator usage. Remember, whenever a function requires an allocator, you have to pass an allocator.

Also, I am questioning the use of config id with string instead of just a number? Then, you do not need to use StringArrayHashMap. A slice with maximum number of config supported with nullable element is enough. This approach will remove a lot of dynamic allocation.

Dynamic allocation is expensive and have a risk of OOM. We want to avoid it as much as we can. Surely, trying to name unique config id with dynamic allocation can be avoided.

solara404 marked this pull request as draft March 31, 2026 07:48

solara404 force-pushed the great_modules_refactor_248 branch from 614715c to 0968336 Compare April 1, 2026 00:18

solara404 force-pushed the great_modules_refactor_248 branch from 7f0518c to 4ebaf9f Compare April 7, 2026 01:37

solara404 marked this pull request as ready for review April 17, 2026 04:10

solara404 force-pushed the great_modules_refactor_248 branch from ee383b4 to d168fe3 Compare April 17, 2026 04:41

solara404 requested a review from aaumar25 April 21, 2026 00:03

solara404 added 8 commits April 21, 2026 16:09

refacator: move from globals to struct

7ea34f9

refactor: switchable configuration

1173027

refactor: change new commands long_description

039fe75

refactor: register and remove commands

0e044c3

refactor: check FilePathTooLong

4de15df

refactor: loading config file without providing .json5 extension

1186e75

refactor: openConfigFile() print fallback information

30cb35b

refactor: add documentation comments

a0ea9d0

aaumar25 reviewed Apr 21, 2026

View reviewed changes

solara404 added 2 commits April 21, 2026 16:36

feat: loadConfig() show already loaded config id and reject config wi…

bfa900d

…th disabled modules

refactor: remove comments, unnecessary steps, debug messages

c9cc14e

refactor: remove module use of return_demo2

b419b93

solara404 force-pushed the great_modules_refactor_248 branch from 94b89e2 to b419b93 Compare April 21, 2026 07:40

aaumar25 requested changes Apr 21, 2026

View reviewed changes

refactor: replacing module_specs EnumArry with reflection

79ce90d

Conversation

solara404 commented Mar 31, 2026

Uh oh!

solara404 commented Mar 31, 2026

Uh oh!

aaumar25 commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

solara404 commented Apr 1, 2026

Uh oh!

solara404 commented Apr 2, 2026

Uh oh!

aaumar25 commented Apr 3, 2026

Uh oh!

solara404 commented Apr 3, 2026

Uh oh!

aaumar25 commented Apr 3, 2026

Uh oh!

solara404 commented Apr 3, 2026

Uh oh!

aaumar25 commented Apr 21, 2026

Uh oh!

aaumar25 commented Apr 21, 2026

Uh oh!

solara404 commented Apr 21, 2026

Uh oh!

aaumar25 Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

solara404 Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

aaumar25 Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

aaumar25 Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

aaumar25 left a comment • edited by solara404 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaumar25 commented Apr 1, 2026 •

edited

Loading

aaumar25 left a comment •

edited by solara404

Loading