Fix NO_GLOBAL_INSTANCES

[paulocsanz]

Fixes #8183

It's not clear to me if this is the best solution. This is just a first draft so the community can debate on a better solution.

But it seems a fairly simple thing to fix. But something needed, if something is used internally it should be declared + defined.

It's not clear to me if we should create it when NO_GLOBAL_STREAMDEV is specifically defined.

[paulocsanz]

Is it better as static or as a regular stack variable? Since it only occupies 1 byte.

[paulocsanz]

@dok-net Can you review it after the Serial changes? I fear it may cause breaking changes since it will force people to define Serial when they didn't have to before.

[dok-net]

ArduinoCore-avr does void serialEventRun(void) differently, I suggest adopting that implementation:
Implement void serialEventRun(void) in HardwareSerial.cpp unconditionally, that is, outside the #if block. In turn, enclose the code inside the function body in the same preprocessor conditional directive.
One could potentially add serialEvent1, too, in the same way.

[paulocsanz]

I've undone the Serial changes, because it seems is a more complex issue that doesn't need to block this PR. We can talk about this more in the issue and if it makes sense, doesn't break compatibility and we find all the other declaration that are not defined we can make a new PR.

[dok-net]

I've undone the Serial changes, because it seems is a more complex issue that doesn't need to block this PR. We can talk about this more in the issue and if it makes sense, doesn't break compatibility and we find all the other declaration that are not defined we can make a new PR.

Declaring symbols that are not defined is an error, don't you think so? Your reversal simply could cause the compile phase to succeed, but then linking will fail. I'm speaking from my perception that it's all about NO_GLOBALS in general, not devnull in particular, of course.

As to my remark involving the extern void serialEventRun(void) __attribute__((weak));, reminding myself that Serial1 is TX-only leaves the existing code as just right. With a little cleanup of a comment, this would be the commit, that you unfortunately just reverted:

diff --git a/cores/esp8266/HardwareSerial.cpp b/cores/esp8266/HardwareSerial.cpp
index 4f09d26f..88b47b33 100644
--- a/cores/esp8266/HardwareSerial.cpp
+++ b/cores/esp8266/HardwareSerial.cpp
@@ -35,9 +35,6 @@

 // SerialEvent functions are weak, so when the user doesn't define them,
 // the linker just sets their address to 0 (which is checked below).
-// The Serialx_available is just a wrapper around Serialx.available(),
-// but we can refer to it weakly so we don't pull in the entire
-// HardwareSerial instance if the user doesn't also refer to it.
 void serialEvent() __attribute__((weak));

 HardwareSerial::HardwareSerial(int uart_nr)
diff --git a/cores/esp8266/HardwareSerial.h b/cores/esp8266/HardwareSerial.h
index bdccae5e..ea17f60e 100644
--- a/cores/esp8266/HardwareSerial.h
+++ b/cores/esp8266/HardwareSerial.h
@@ -233,8 +233,12 @@ protected:
     size_t _rx_size;
 };

+#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
 extern HardwareSerial Serial;
+#endif
+#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL1)
 extern HardwareSerial Serial1;
+#endif

[paulocsanz]

I added it again

[earlephilhower]

Thanks for trying to fix multiple things, but it’s really neat if you do one change per PR, please. Can you remove the unrelated hardware serial stuff?

[dok-net]

Thanks for trying to fix multiple things, but it’s really neat if you do one change per PR, please. Can you remove the unrelated hardware serial stuff?

@earlephilhower If it were a mistake, it would be on me, I've asked the submitter to add it back in. You would have to try and see it from a different angle: The issue is not to create the devnull, but the fact that NO_GLOBAL_INSTANCES is not handled correctly. This applies to both HardwareSerial and HttpClient. Plus, the comment that got removed is non-applicable to ESP8266's implementation and confusing, which turns up when doing this change.

There are NO further issues in this core regarding NO_GLOBAL_INSTANCES!

So, isn't it best to rename this PR, instead of splitting it up into extraordinarily small chunks? Besides, it's a simple fix for HardwareSerial, nothing else, or what could possible be the point in declaring something extern (no weak or anything) in the core headers that is not defined? It's just going to produce a linker error, which are harder on the average coder to understand.

[paulocsanz]

Awaiting further instructions. I'm OK with both approaches, just ping me and I will edit the PR accordingly (or make a new PR to split the changes). But I don't have much to contribute to the discussion. I agree that this could become a PR to fix all the problems related to NO_GLOCAL_INSTANCES, but I haven't ensured those are the only ones, they are the ones I found while maintaining our system.

[dok-net]

@paulocsanz I grepped all cores and libraries and found no other uses, that's all.

[earlephilhower]

OK, I'm convinced. LGTM.