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 std. "ESP32 Dev Module" #110

Closed rotorman closed 11 months ago

rotorman commented 11 months ago

Added std. "ESP32 Dev Module" (e.g. NodeMCU ESP32-WROOM-32) as a new option. Including also an inverted serial version for usage e.g. with FrSky R9M.

According to Discord user hooman, this code works with his R9M and NodeMCU ESP32-WROOM-32.

Update: tested myself with ESP32-DevKitC V4 with ESP32-WROOM-32D as well.

olliw42 commented 11 months ago

first, many thx on a first quick inspection, I have three comments

olliw42 commented 11 months ago

this lgtm, but let it rest for a bit before I merge, in case more thoughts come up.

very nice

olliw42 commented 11 months ago

I guess I'm going to repeat myself

this lgtm, but let it rest for a bit before I merge, in case more thoughts come up.

very nice, MANY thx so far, really much appreciated

olliw42 commented 11 months ago

@brad112358 in case you have the chance, could you test this PR (afterthe open review has been done) on your R9M ESP32 setup? I just like to be sure we haven't broken it.

IJdewangan commented 11 months ago

i am getting an error while uploading code in NodeMcu can anyone help? Code in flash (default, ICACHE_FLASH_ATTR), used 232100 / 1048576 bytes (22%) ║ SEGMENT BYTES DESCRIPTION ╚══ IROM 232100 code in flash

rotorman commented 11 months ago

Do you have ESP32 or ESP8266 NodeMCU? Which board did you select in Arduino IDE? For ESP32-WROOM-32 it should be Boards -> esp32 -> ESP32 Dev Module.

brad112358 commented 11 months ago

I'm not sure why we need the #undef USE_SERIAL... everywhere since they will be undefined by default. Given we now have 3 options for USE_SERIAL..., would it make more sense to eliminate all 3 and just move the SERIAL and DBG defines into the board details #if defined blocks? Then the DBG_PRINT(x) can use the DBG macro and be conditional on it being defined. This all results in a bit less code (especially with the #undef USER_SERIAL...) and also easily allows the remaining combinations of Serial, Serial1, ... for future boards.

brad112358 commented 11 months ago

Another thought.... The current PR seems to require a change to the R9 documentation we just updated to mention the need to define USE_SERIAL_INVERTED. That's fine, but then we may as well remove the "for Frsky R9M" references everywhere. Alternatively, we could add "#define USE_SERIAL_INVERTED" to the "For Frsky R9M" boards and move the #define for this down in the .ino file to after the #include "mlrs-wifi-bridge-boards.h"

olliw42 commented 11 months ago

the intention of the flags is to be able to define your properties entirely in the main sketch, i.e., the .ino file, if the pre-prepared modules don't work for you. Hence in the boards.h you cannot rely on what you think are "defaults". I would like to keep it that way. yes, for the R9M variants, the flag in the boards.h file needs to be changed to USE_SERIAL_INVERTED as you indicate, pl see the pending review which Risto hasn't yet accounted for. We could change things for the R9M cases, but I am happy to have it easy for a user for these two specific boards. My main concern at this point is that we haven't broken anything.

brad112358 commented 11 months ago

Perhaps I'm missing something. I don't see any review comments not marked resolved.

olliw42 commented 11 months ago

oh damed ... I did it again ... grrrrrrr THX for mentioning!

olliw42 commented 11 months ago

MANY THX sorry for having forgotten to finalize the pending review :( lgtm I will wait a bit to see if there is some confirmation/disconfirmation from Brad if it still works for him.

brad112358 commented 11 months ago

Yes, I tested both M5STAMP_C3U_MATE_FOR_FRSKY_R9M and MODULE_M5STAMP_PICO_FOR_FRSKY_R9M and they worked as expected once I remembered that I needed to change the !#*@&% channel from 13 to something that works in North America.

I don't usually need to remember this since I'm stuck maintaining my own branch anyway (because I want to pack an entire MAVLink messages in a single UDP packet to allow Wireshark to decode it), but it might be nicer especially for new users not to have to mess with this.

Does using channel 13 still bring a significant benefit for mLRS users on 2.4 GHz? Or do I remember seeing there is now a way to avoid an arbitrary WiFi channel? If we don't want to change the default channel to something that works everywhere, I suppose I should update the R9 doc as I forgot to mention it there.

olliw42 commented 11 months ago

Yes, I tested both M5STAMP_C3U_MATE_FOR_FRSKY_R9M and MODULE_M5STAMP_PICO_FOR_FRSKY_R9M and they worked as expected

very cool. MAYN THX. So I will merge.

Does using channel 13 still bring a significant benefit for mLRS users on 2.4 GHz? Or do I remember seeing there is now a way to avoid an arbitrary WiFi channel? If we don't want to change the default channel to something that works everywhere, I suppose I should update the R9 doc as I forgot to mention it there.

Yes, mLRS has the capability now to exclude specific bands, see https://github.com/olliw42/mLRS-docu/blob/master/docs/PARAMETERS.md#bind-phrase. This is - at least for me, unfortunately no reports - very situation dependent however, and the "best" needs to be determined by trial and error. I e.g. exclude #6. And it is not a 100% perfect solution anyway, I believe because of receiver desensitation, it just mitigates the issue significantly.

Anyway, if 13 is not an option in the US, we should choose something which is. 13 was meanigfull since it's really at the end of the fhss table. Moreover, for all those not on 2.4 GHz it doesn't matter, so one could argue that the 2.4 GHz folks should carry the burden. Any suggestion instead? Pl raise a PR.

olliw42 commented 11 months ago

@rotorman @brad112358 MAY THX for this. I'm merging.