TTGO T-Beam pinmap - Githubissues

matthijskooijman commented 4 years ago

Thanks for your contribution. Do you have a link to documentation of these boards? Also, it seems that there are two different boards with different radios, how does that work? Are they separate boards in the Arduino IDE, or do you just use the same board selection for both? It might be nicer to check for each specific board if possible, to prevent a situation where you're using the SX1276 version but have BRD_sx1262_radio defined, which is not caught at compiletime now I think.

matthijskooijman commented 4 years ago

Also, it seems that the define used is actually problematic with older compilers. That is, the autocompile files for STM32 and ESP8266, which I think use an older compiler. They fail with:

examples/basicmac-otaa/standard-pinmaps.ino:90:24: error: missing ')' after "defined"
 #elif defined(ARDUINO_T-Beam)
                        ^

I wonder if that define (with a -) is a valid define at all?

lyusupov commented 4 years ago

I wonder if that define (with a -) is a valid define at all?

they use it with Arduino Core for ESP32: https://github.com/espressif/arduino-esp32/blob/master/boards.txt#L3548

if it fails with some toolchains - it would probably make sense to add extra #ifdef ESP32 brackets ?

lyusupov commented 4 years ago

Do you have a link to documentation of these boards?

The board's primary (GitHub) support page is this one: https://github.com/Xinyuan-LilyGO/LilyGO-T-Beam

in the 'schematic' folder you will be able to see PDF schematic files of every major T-Beam board revision.

Also, it seems that there are two different boards with different radios, how does that work?

The dev. board is using few pads on it's PCB to solder a 'radio module'. Pinout for the radio module(s) was choosen to be compatible across both SX127x (HPD13A) and SX126X (HPD16A) variants of the module.

Are they separate boards in the Arduino IDE, or do you just use the same board selection for both?

They manufacture the board since 2018. The board had the only SX127x radio until now. SX126x variant of the board is currently available in samples but is expected to be listed for sale on AliExpress within this July.

So, official Arduino Core for ESP32 'knows' about SX127x variant only at this time.

matthijskooijman commented 4 years ago

Thanks for the additional clarifications!

I wonder if that define (with a -) is a valid define at all?

The gcc docs suggest it is not valid:

An identifier is the same as an identifier in C: any sequence of letters, digits, or underscores, which begins with a letter or underscore.

I also tried this macro in a few compilers (mainline amd64 gcc, but also the esp32 compiler) and none of them actually accept it. e.g.:

~/.arduino15/packages/esp32/tools/xtensa-esp32-elf-gcc/1.22.0-80-g6c4433a-5.2.0/bin/xtensa-esp32-elf-gcc esp32.cpp 
esp32.cpp:1:22: error: missing ')' after "defined"
 #if defined(ARDUINO_T-Beam)
                      ^
esp32.cpp:1:23: error: missing binary operator before token "Beam"
 #if defined(ARDUINO_T-Beam)
                       ^

The reason this seems to work with some platforms but not others seems to be that when used like below (e.g. when the invalid macro is not actually used because an earlier block is used), newer compilers do not even try to parse the invalid macro (so compilation works), while older compilers parse everything (and fail):

#define FOO
#if defined(FOO)
#elif defined(ARDUINO_T-Beam)
#endif

But that means that the defined(ARDUINO_T-Beam) only "works" when it is not used, when actually compiling for the T-Beam board, you'll get an error anyway.

Did you actually test that loading the pinmap from standard-pinmaps.ino works for your board? Or did you just test with a custom pinmap and then copy it into standard-pinmaps.ino?

All this makes me wonder if this macro is usable at all? I have the idea that maybe nobody tried to check for it so far (and whoever decided on the value just used a - without realizing that this was problematic). If so, this should be fixed in the ESP32 core first, I think.

So, official Arduino Core for ESP32 'knows' about SX127x variant only at this time.

Ah, right, so I guess your proposed approach is then fine (at least until the ESP32 core gets a separate board entry, if ever).

lyusupov commented 4 years ago

A PR to Arduino Core ESP32 with initial support for the T-Beam board had the 'dash' in the 'board name' from very begining: https://github.com/espressif/arduino-esp32/pull/1852/commits/c70d59b40c770230fefa8699ad378ed203258217

Author of the PR has recently responded to me that he is unable to make a fix due to no response from Arduino Core for ESP32 developers/maintainers:

Hi, Linar

Sorry, this is my mistake. I have the same mistake in T-Watch. I have submitted a request to Esp32_Core last year, but I did not get their permission. I have no way to change the board selection to 32Dev. ?

If we will take a look into PR processing queue on the AC/ESP32 GutHub page: https://github.com/espressif/arduino-esp32/pulls?q=is%3Apr+is%3Aclosed we will see that most of the PRs has been closed with little or no processing. I suspect that core developer is either very busy or left the project.

My question to you is:

Is it Ok for you if I will:

1) add one more patch to this PR where 'ARDUINO_T-Beam' is substituted by 'ARDUINO_TBeam' , and 2) create a separate PR onto AC/ESP32 with a fix for t-beam.build.board variable definition in boards.txt file.

The 1st action should fix the BASICMAC build failure. However, it will not make the patch effective since the ARDUINO_TBeam macro was never defined by current AC/ESP32. When, eventually, the AC/ESP32 guys will accept my PR in the 2nd action - the patch will become fully functional at that time.

matthijskooijman commented 4 years ago

I have submitted a request to Esp32_Core last year, but I did not get their permission.

I read this in your forwarded e-mail, but I'm unsure what it means. Searching for "beam" in the list of pullrequests does now show any PRs that try to fix this particular problem.

we will see that most of the PRs has been closed with little or no processing. I suspect that core developer is either very busy or left the project.

Are you sure? The few closed PRs I looked at were reasonably closed (not an actualy PR, submitter changed their mind, wrong repository to fix something, fixed in another PR already, etc.). Seeing that some PRs are actually being merged is probably a better indication of activity.

Regarding your proposal that seems good to me. However:

What is "AC/ESP32" exactly? Is that another repository than the one you linked, or?
Good to update this PR already, but I probably won't merge it until the ESP32 core is fixed (otherwise it suggests that it works, but won't, which is only confusing to people).

lyusupov commented 4 years ago

I am monitoring activity of the Arduino Core for ESP32 (AC/ESP32) development since 2017.

Leading developer and 'driving force' of the project was always this guy: https://github.com/me-no-dev

After he released 1.0.4 in October of 2019: https://github.com/espressif/arduino-esp32/releases he gradually reduced his activity with Arduino ESP32 port development down to a bare minimum and I see no more of actions (except very few) he did since begining of 2020.

These is still some 'buzz' activity remaining there but I see no one that could be as responsible and effective as this guy did.

lyusupov commented 3 years ago

Fortunately, a bunch of PRs has been accepted into Arduino Core for ESP32 on Sep 30, Oct 1 and 3. Board name change for the TTGO T-Beam is one of these PRs been merged: https://github.com/espressif/arduino-esp32/pull/4212

I've updated this pull request to match that fixed board definition.

We can see that:

it no longer breaks Lacuna BASICMAC GitHub CI build ;
"... the ESP32 core is fixed ... "

matthijskooijman commented 3 years ago

Thanks for the update. However, since you started this PR, the handling of TCXO and RXTX for the 1262 has changed, you must now explicitly enable DIO-control of these in the pinmap. See e.g.

https://github.com/LacunaSpace/basicmac/blob/35a3f65cc7d9bb52ddfdf31b3b258ad3882a2536/target/arduino/examples-common-files/standard-pinmaps.ino#L89

https://github.com/LacunaSpace/basicmac/blob/35a3f65cc7d9bb52ddfdf31b3b258ad3882a2536/target/arduino/examples-common-files/standard-pinmaps.ino#L95

Could you:

Rebase on top of current master
Add the changes to the pinmap for DIO-control as shown above
Ideally, merge all changes into a single commit
Re-test that the result still works?

If you're unsure how, I can also make the change, but I can't test the result for lack of hardware.

With those changes applied, I think this can be merged.

lyusupov commented 3 years ago

T-Beam's .tx and .tcxo pins map has been updated

I confirm that it works on the TTGO hardware.

matthijskooijman commented 3 years ago

I see you haven't rebased on top of master yet, but I assume you tested this pinout against the current master version of the library (otherwise the DIO-control constants won't work, of course).

I did the remaining changes (rebase and squash commits), added a few more lines of comment, and force-pushed to your branch to update this PR. I'll merge it next.

LacunaSpace / basicmac

TTGO T-Beam pinmap #15