Remove Global State

[KillerInk]

This PR removes reliance on the GlobalState structure to improve readability, maintainability, and functionality.

Removed the GlobalState parameter from most functions, simplifying function signatures.
Extracted relevant data directly where needed instead of relying on a global state.
These changes aim to streamline the ASIC management code while ensuring all ASIC types are properly supported and making the codebase more modular.

[github-actions]

Test Results

20 tests  ±0   20 ✅ ±0   0s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 378bd14. ± Comparison against base commit bbddfdc.

♻️ This comment has been updated with latest results.

[johnny9]

NACK

This doesn't address the issue with the dependancies between these components and main and in fact just makes a lot of this harder to eventually test.

What needs to be done is asic.h, for instance, needs to no longer know about global state.

On top of that this is far too large of a change to go in without any additional tests being written.

[KillerInk]

NACK

This doesn't address the issue with the dependancies between these components and main and in fact just makes a lot of this harder to eventually test.

What needs to be done is asic.h, for instance, needs to no longer know about global state.

On top of that this is far too large of a change to go in without any additional tests being written.

the thing is that the GLOBAL_STATE is static in main but not accessible without routing it through methods, in fact its static^^
other thing is that everything that is stored in that pointer struct can lead to mem leaks when a new value get set and the old one is not freed.

ESP-Miner/main/tasks/stratum_task.c

Lines 343 to 346 in d47275b

	char * old_extranonce_str = GLOBAL_STATE->extranonce_str;
	GLOBAL_STATE->extranonce_str = stratum_api_v1_message.extranonce_str;
	GLOBAL_STATE->extranonce_2_len = stratum_api_v1_message.extranonce_2_len;
	free(old_extranonce_str);

like this
this makes sure that every struct with extern flag can exists only once.
i can also move the structs into own headers, then they can get included as needed

[johnny9]

the thing is that the GLOBAL_STATE is static in main but not accessible without routing it through methods, in fact its static^^ other thing is that everything that is stored in that pointer struct can lead to mem leaks when a new value get set and the old one is not freed.

Ok, this might be worthwhile and I might agree, but you called the PR splitt modules but there wasn't any change in dependencies. You need to work on the commit and PR clarity here. It changes 1000 lines of code. Lets be clearer on the intent

[johnny9]

other thing is that everything that is stored in that pointer struct can lead to mem leaks when a new value get set and the old one is not freed.

ESP-Miner/main/tasks/stratum_task.c

Lines 343 to 346 in d47275b

	char * old_extranonce_str = GLOBAL_STATE->extranonce_str;
	GLOBAL_STATE->extranonce_str = stratum_api_v1_message.extranonce_str;
	GLOBAL_STATE->extranonce_2_len = stratum_api_v1_message.extranonce_2_len;
	free(old_extranonce_str);

like this

Not everything in the struct will cause memory leaks. Only the alloc'd values like the strings. That is unavoidable. we need to free the dynamically allocated strings regardless of if GLOBAL_STATE holds the pointer or some other module.

[KillerInk]

other thing is that everything that is stored in that pointer struct can lead to mem leaks when a new value get set and the old one is not freed.

ESP-Miner/main/tasks/stratum_task.c

Lines 343 to 346 in d47275b

	char * old_extranonce_str = GLOBAL_STATE->extranonce_str;
	GLOBAL_STATE->extranonce_str = stratum_api_v1_message.extranonce_str;
	GLOBAL_STATE->extranonce_2_len = stratum_api_v1_message.extranonce_2_len;
	free(old_extranonce_str);

like this

Not everything in the struct will cause memory leaks. Only the alloc'd values like the strings. That is unavoidable. we need to free the dynamically allocated strings regardless of if GLOBAL_STATE holds the pointer or some other module.

i did not say it will, it can. but its now more safe because its using the same memory location and make the free obsolete.
on my side i never noted that leak^^

the thing is that the GLOBAL_STATE is static in main but not accessible without routing it through methods, in fact its static^^ other thing is that everything that is stored in that pointer struct can lead to mem leaks when a new value get set and the old one is not freed.

Ok, this might be worthwhile and I might agree, but you called the PR splitt modules but there wasn't any change in dependencies. You need to work on the commit and PR clarity here. It changes 1000 lines of code. Lets be clearer on the intent

I probably have to, if you have a good title feel free to change it

[KillerInk]

i rechecked everything requested

[mutatrum]

All these includes is a sign that we need to overhaul this class as well. But that's outside the scope of this PR.

[mutatrum]

Interesting to see this moved here. Kinda makes sense, at least in the current version.

[mutatrum]

Similar commend to http_server, this file might need to be broken up as well. But not for this PR.

[mutatrum]

This comment is not correct I think. AFAIK it has something to do with the statistics. Maybe those can be moved into a Statistics_module?

[KillerInk]

its correct^^

ESP-Miner/main/system.c

Line 213 in 27c8dc4

double duration = (double) (esp_timer_get_time() - module->duration_start) / 1000000;

[KillerInk]

its only used in system.c it dont have to be global

[mutatrum]

As an exercise to the reader (me) I tried to only make GLOBAL_STATE static, and having done that, I'm not sure if this is the right direction to go. It makes testing harder, as you can't easily inject mock or test data anymore. To be able to do this, we would need to revert back to accepting parameters again. Furthermore, it makes lazy programming easier, by not having to think about responsibilities of a function, as everything is available everywhere.

My try is in https://github.com/mutatrum/ESP-Miner/tree/static-global-state. Some stuff could be done further, f.e. the BM####_init functions don't need these parameters anymore, as the bm####.c files can access them directly. And that's exactly where it gets tricky IMO.

Having said that, I think the changes on how the modules are separated do make sense. Maybe a better way would be to indeed remove GlobalState, have the modules live in main.c and only pass around the specific modules that functions need. This forces a better encapsulation of responsibilities.

[KillerInk]

this is only the top of the iceberg^^
the main problem are the cross references. class a need b and b need a.
dont know but maybe things should get a little bit more modular.
as sample the stratum and asic task.
stratum does one thing with the asic task, its sends a new notification to asic. and asic send the result to stratum
that could get done different with function callbacks. then both classes dont rely on each other and you can easly replace the asic task with a different, if needed, without touching the stratum code or vise versa.
i think most the of the module structs can get removed/replaced by a local usage with a more modular approach.
that could also help to simplyfy and improve testing and reduce code complexity and force a better encapsulation of responsibilities^^

https://github.com/KillerInk/ESP-Miner/blob/385739054fc73e7516e0fc36e39533da44d010de/main/main.c#L91-L92

https://github.com/KillerInk/ESP-Miner/blob/8f43e63d151c35542534d4719d2a6153c4e7ddc5/main/tasks/asic_task.h#L12-L13
https://github.com/KillerInk/ESP-Miner/blob/8f43e63d151c35542534d4719d2a6153c4e7ddc5/main/tasks/asic_task.c#L29-L30
https://github.com/KillerInk/ESP-Miner/blob/8f43e63d151c35542534d4719d2a6153c4e7ddc5/main/tasks/asic_task_common.c#L27-L31

https://github.com/KillerInk/ESP-Miner/blob/8f43e63d151c35542534d4719d2a6153c4e7ddc5/main/tasks/stratum_task.h#L8
https://github.com/KillerInk/ESP-Miner/blob/8f43e63d151c35542534d4719d2a6153c4e7ddc5/main/tasks/stratum_task.c#L33
https://github.com/KillerInk/ESP-Miner/blob/8f43e63d151c35542534d4719d2a6153c4e7ddc5/main/tasks/stratum_task.c#L336