olliw42 / mLRS

2.4 GHz & 915/868 MHz & 433 MHz/70 cm LoRa based telemetry and radio link for remote controlled vehicles
GNU General Public License v3.0
279 stars 58 forks source link

Added mLRS-Wireless-Bridge for ESP8266 #118

Closed rotorman closed 1 month ago

rotorman commented 11 months ago

As ESP8266 cannot do Bluetooth, this edition supports WiFi only.

Tested with ESP-01S, but needs more testing.

olliw42 commented 11 months ago

very nice. I like that, and I like you put the esp8266 into an extra file.

(although I wonder if there wouldn't be a neat and clean way to do it)(clean != lots of define dirt in the main ino)

however, please don't rename the esp32 folder&files. Few days ago it all was slim and comprehensible, but now it's getting somewhat out of hands :):) also, remind, each such change induces a chain of actions in the docs ... the docs still don't even reflect the last change fully!!! let's pl stick with mlrs-wireless-bridge for the EPS32 branch :)

rotorman commented 11 months ago

OK, reverted the folder and file name change. It surely would be possible to have only a single *.ino file, but there is IMO no way around having some #ifdefs to differentiate between ESP32 and ESP8266.

olliw42 commented 11 months ago

THX.

one point which really concerns me is that it will lack maintenance, since I don't think many will use it and especially not many of the already few who will use it will work on the code to keep it updated ... I am not sure how we should handle that, but I think it needs a scheme since what will be sure, I will not take care of that code. You would have an idea? Only two thoughts crossed my mind (neither necessarily good)

rotorman commented 11 months ago

Done with the file and folder rename. I do not need the credit line. The commit below adds also a comment about unmaintained code.

Before merging, would be nice to have at least one user provide feedback about success of using this code.

olliw42 commented 11 months ago

Before merging, would be nice to have at least one user provide feedback about success of using this code.

would be nice ... but may potentially delay merge to indefinite

olliw42 commented 11 months ago

lgtm from my side. MANY THX!

concenring "before merging, would be nice to have at least one user provide feedback", pl drop a note when you want it to get merged :)

rotorman commented 11 months ago

I had a look at how to integrate ESP-01S support into existing mlrs-wireless-bridge and it does not look bad: https://github.com/rotorman/mLRS/commit/7db29ef33b9027735a2d75fa48d04152ed78796f Should we close this here and I re-open a new PR with integrated ESP8266 support?

Note that the rename of SERIAL to SERIALx stems from the issue that ESP8266 compiler does not seem to differentiate between upper and lower case, thus #define SERIAL Serial gave me a build error.

olliw42 commented 11 months ago

I was briefly looking at this too, and it indeed doesn't look too bad ... but:

I am hesitant.

I guess we can keep it as it is. It's good to know however how it woudl look like in one file.

that ESP8266 compiler does not seem to differentiate between upper and lower case

I frankly find that hard to believe. I would think that there is probably another issue at work.

rotorman commented 11 months ago

I frankly find that hard to believe. I would think that there is probably another issue at work.

I you have an idea what else it could be, would be interested to learn it.

olliw42 commented 11 months ago

it's just that I never have come across a C compiler which would be case insensitive my first guess would be that some library defines it already itself

olliw42 commented 1 month ago

many THX !!!