-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
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
Cleanup ahead of MeatPack refactor #21308
Cleanup ahead of MeatPack refactor #21308
Conversation
1362f7f
to
28803fa
Compare
Marlin/src/HAL/DUE/HAL.h
Outdated
typedef ForwardSerial1Class< decltype(Serial1) > DefaultSerial2; | ||
typedef ForwardSerial1Class< decltype(Serial2) > DefaultSerial3; | ||
typedef ForwardSerial1Class< decltype(Serial3) > DefaultSerial4; | ||
extern DefaultSerial1 MSerial; |
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.
While fixing a bug with teensy, I was wondering: why not call those MSerial0 directly ? So that we can preprocessor them with the serial port:
#define DECLARE_SERIAL(X) typedef XXX<decltype(Serial##X)> DefaultSerial##(INCREMENT(X)); extern DefaultSerial##INCREMENT(X) MSerial##X
// In effect, let's have a MSerial0, MSerial1 and so on.
DECLARE_SERIAL(SERIAL_PORT)
Let's drop Arduino dumbness about the first serial being different so that we always have prefix ## index here.
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.
Where we have control over the name, we can certainly add the zero. I was superstitious that such a simple name must come from a platform, but now I think the M stands for "Marlin."
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.
Meanwhile, the Arduino-sketchy names "MYSERIAL0/1
" are seeming kind of quaint now. Any of SERIAL(n)
or MSERIAL(n)
or maybe MFSERIAL(n)
would be better.
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.
Even a bit more hardcore, but what about #define Serial0 Serial
and be done with special case for 0 ?
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.
There are so many X Serial Y in the code base now, my head hurts...
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.
If the composition is working as expected, there should be only one SERIAL_IMPL in the code base (except for HAL), the rest should be hidden underneath the carpet. So let's simplify all special cases here so we can preprocess as much as we can and remove all special case that are not preprocessor friendly, IMHO.
28803fa
to
00cda52
Compare
Now you can do: diff --git a/Marlin/src/HAL/TEENSY35_36/HAL.cpp b/Marlin/src/HAL/TEENSY35_36/HAL.cpp
index 547681de5f..8640bdfe00 100644
--- a/Marlin/src/HAL/TEENSY35_36/HAL.cpp
+++ b/Marlin/src/HAL/TEENSY35_36/HAL.cpp
@@ -31,7 +31,12 @@
#include <Wire.h>
-DefaultSerial MSerial(false);
+#define _IMPLEMENT_SERIAL(X) DefaultSerial##X MSerial##X(false)
+#define IMPLEMENT_SERIAL(X) _IMPLEMENT_SERIAL(X)
+#if WITHIN(SERIAL_PORT, 0, 3)
+ IMPLEMENT_SERIAL(SERIAL_PORT);
+#endif
+
USBSerialType USBSerial(false, SerialUSB);
uint16_t HAL_adc_result, HAL_adc_select;
diff --git a/Marlin/src/HAL/TEENSY35_36/HAL.h b/Marlin/src/HAL/TEENSY35_36/HAL.h
index f77b71d91c..85f2071a75 100644
--- a/Marlin/src/HAL/TEENSY35_36/HAL.h
+++ b/Marlin/src/HAL/TEENSY35_36/HAL.h
@@ -54,18 +54,24 @@
#endif
#include "../../core/serial_hook.h"
-typedef Serial1Class<decltype(Serial)> DefaultSerial;
-extern DefaultSerial MSerial;
+
+#define _DECLARE_SERIAL(X) \
+ typedef Serial1Class<decltype(Serial##X)> DefaultSerial##X; \
+ extern DefaultSerial##X MSerial##X;
+#define DECLARE_SERIAL(X) _DECLARE_SERIAL(X)
+
+
typedef ForwardSerial1Type<decltype(SerialUSB)> USBSerialType;
extern USBSerialType USBSerial;
#define _MSERIAL(X) MSerial##X
#define MSERIAL(X) _MSERIAL(X)
-#define MSerial0 MSerial
+#define Serial0 Serial
#if SERIAL_PORT == -1
#define MYSERIAL1 USBSerial
#elif WITHIN(SERIAL_PORT, 0, 3)
+ DECLARE_SERIAL(SERIAL_PORT)
#define MYSERIAL1 MSERIAL(SERIAL_PORT)
#endif
So we don't even care of the DefaultSerialXX number here (not used anywhere anyway, except for the instantiation), only the preprocessor need to know it. (And it also fix the missing definition&declaration of MSerial1 and consort in the previous code). |
Some of those changes refer to code from the future. |
That line goes away, so this is probably bunk. |
When you'll have merged those, I'll submit another PR for fixing some teensy building issue with SERIAL_PORT defined to 1 or more. That should fix some reported bugs #21277 |
For the most part this is all cosmetic. After this is merged, I will do a merge to your PR, leaving only the relevant MeatPack changes visible. That would be a good point to add more commits, or just wait for that to be merged and start a new PR from the bugfix at that point. |
That's for the |
Yes, sure. Code is a lot easier to read when there is not so many special case. |
Bah, please revert last 2 commit, I'll fix them later on cleanly. I don't want to slow you down. |
3a82243
to
49ec52b
Compare
Would I have gotten this far without helpful hints from the gallery? Now we will never know. 😉 |
The boring parts of #21306…
MEATPACK
test for changed configuration.SERIAL_PORT
(1) /SERIAL_PORT_2
.Platform-based low-level serial port wrappers and hardware may index from 0.
That's fine. Marlin's serial wrapper things are de facto 1-based … and can go to eleven.