JAndrassy / ArduinoOTA

Arduino library to upload sketch over network to Arduino board with WiFi or Ethernet libraries
GNU Lesser General Public License v2.1
435 stars 89 forks source link

Make ArduinoOTA definitions `extern` in header #251

Closed RedSponge closed 2 months ago

RedSponge commented 2 months ago

Having the definitions be non-extern meant that the header couldn't be included by multiple files at once.

This patch fixes the problem but may not be the best choice for a solution - perhaps a singleton would be better fit? It may be a better design choice but will cause an API break.

JAndrassy commented 2 months ago

it will not work this way. the defines don't exists at cpp compilation

RedSponge commented 2 months ago

It worked for me in a cpp project.. Perhaps I misunderstand your comment - which defines don't exist and what fixes should I make?

JAndrassy commented 2 months ago

It worked for me in a cpp project.. Perhaps I misunderstand your comment - which defines don't exist and what fixes should I make?

none of etherneth, ethernet_h, UIPETHERNET_H, WIFIESPAT1 use in .h to calculate default OTETHERNET and NO_OTA_PORT OTETHERNET and NO_OTA_PORT wouldn't be defined too of course

so it will always build the cpp with extern ArduinoOTAMdnsClass <WiFiServer, WiFiClient, WiFiUDP> ArduinoOTA;

RedSponge commented 2 months ago

So what you're saying is I should only keep the extern on the Mdns class, and revert the rest?

RedSponge commented 2 months ago

Also - will it always be the case that these other classes are only used in a C context? Won't it be better to future proof them for the day the defines change and expose a different one of them?

JAndrassy commented 2 months ago

So what you're saying is I should only keep the extern on the Mdns class, and revert the rest?

I say the library would be broken. It would only work for some WiFi libraries. and it would ignore all options set with defines

RedSponge commented 2 months ago

I'm not sure I understand, why would adding the externs break the library? How does that affect the defines? The current state of the library is broken when including in multiple c/cpp files, and I don't see what that has to do with the defines..

As far as I understand, all extern does is tell the compiler that the variables are defined somewhere else, and that way only one CPP file defines them and the collision is avoided

Can you please explain why this change affects the defines?

JAndrassy commented 2 months ago

just try it with the Ethernet library (OTEthernet example). you will get a linker error. the cpp will have OTA object for WiFi and the extern will expect a OTA for Ethernet

EDIT: it doesn't even compile as it is. I get "ArduinoOTA.h:141:29: error: 'WiFiServer' was not declared in this scope" (because there is no networking library included in ArduinoOTA.cpp)

JAndrassy commented 2 months ago

you can do something like this: in your sketch folder create ota.h

#pragma once

#include <Arduino.h>
#include <Ethernet.h>
#define NO_OTA_NETWORK
#include <ArduinoOTA.h>

extern ArduinoOTAMdnsClass  <EthernetServer, EthernetClient, EthernetUDP>   ArduinoOTA;

and ota.cpp

#include "ota.h"

ArduinoOTAMdnsClass  <EthernetServer, EthernetClient, EthernetUDP>   ArduinoOTA;