-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code refactoring to make the code easier to read #671
Conversation
c0fb6b7
to
d580d3f
Compare
#define CRYSTAL_FREQUENCY_MHZ 8 //CAN_ADDON option, what is your MCP2515 add-on boards crystal frequency? | ||
//#define CANFD_ADDON //Enable this line to activate an isolated secondary CAN-FD bus using add-on MCP2518FD chip / Native CANFD on Stark board | ||
#ifdef CANFD_ADDON // CANFD_ADDON additional options if enabled | ||
#define CANFD_ADDON_CRYSTAL_FREQUENCY_MHZ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CANFD_ADDON_CRYSTAL_FREQUENCY_MHZ could always be defined, irregardless of CANFD_ADDON setting (makes this file smaller and easier to manage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing this change is fine by me, to make an upgrade to the code. In this PR I have tried to just refactor the code, without introducing changes to how the code works.
Shall we keep the suggested change for another PR?
Very nice refactor, the silly large Software.ino refactor has been on my mind for a while, but been so busy with new integrations 😅 |
@lenvm I think we can start looking into merging this, I just applied the critical PWM fix to main, and tagged 7.9.1 (so sorry, you'll have to deal with a minor merge conflict) |
431e4b0
to
fdf2516
Compare
@dalathegreat, no worries, I have rebased and resolved the merge conflict. Please let me know if we need to merge #673 too, before we merge this one. |
I have merged #673, but I do not see your rebase and the conflict is unsolved. If you commit the rebase, I will review and approve. Better not to keep a large PR like this around too long. |
…ated function to seriallink folder
…ntactorcontrol folder
…ipmentstopbutton folder
fdf2516
to
a9da915
Compare
a9da915
to
4e3dcf1
Compare
@dalathegreat, @mvgalen, I have rebased on main again. Please review the changes now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Well needed refactoring! 🙌
What
This PR splits
Software.ino
into a number of different files.It also updates:
DUAL_CAN
toCAN_ADDON
CAN_FD
toCANFD_ADDON
Why
How
Different folders are created, each containing a specific subset of the code:
can
: for functions related to CANcontactorcontrol
: for functions related to contactor controlequipmentstopbutton
: for functions related to the equipment stop buttonnvm
: for functions related to setting storagers485
: for functions related to RS485 / ModBusseriallink
: for functions related to Serial LinkIn case you have any comments or requests, please let me know!