Refactor ASIC Initialization, Frequency Management, and Remove Global State #1146
Conversation
There was a problem hiding this comment.
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^^ ESP-Miner/main/tasks/stratum_task.c Lines 343 to 346 in d47275b 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 |
…s from process work that they match the header declaration
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 |
components/asic/asic.c
Outdated
| #include "asic.h" | ||
| #include "device_config.h" | ||
| #include "power_management_task.h" |
There was a problem hiding this comment.
damn this sucks. I know you didn't cause the problem and this change is just exposing it more but asic shouldnt care at all about the tasks that are leveraging it.
There was a problem hiding this comment.
there will more come up. its not the first time im doing such things^^
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.
I probably have to, if you have a good title feel free to change it |
# Conflicts: # main/global_state.h # main/tasks/asic_result_task.c # main/tasks/power_management_task.c
|
Concept ACK. This is starting to look much better. I'll make an effort to test this out this week and do a more thorough review. |
|
Take your time. i dont expect this get merged soon^^ |
# Conflicts: # main/http_server/http_server.c # main/tasks/power_management_task.c
This PR refactors the ASIC initialization and frequency management code, and removes reliance on the GlobalState structure to improve readability, maintainability, and functionality.
Key changes include:
Function Consolidation: Merged redundant functions and improved function organization.
Global State Removal:
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.