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
295 stars 67 forks source link

Clarification | E5 grove with out #54

Closed jlpoltrack closed 1 year ago

jlpoltrack commented 1 year ago

To enable CRSF out on the E5 grove, I add the line '#define DEVICE_HAS_OUT' to the HAL (wire is soldered to PC1), but this seems to output CRSF at 100K baud which ArduPilot doesn't recognize. When I edit the line '#define UART_BAUD' line to 416666 (instead of 100000) all is good. Is this expected behavior? Thanks.

olliw42 commented 1 year ago

thx for the report no, this is supposed to work: https://github.com/olliw42/mLRS/blob/main/mLRS/CommonRx/mlrs-rx.cpp#L93-L100 I have briefly looked into the code, but couldn't see why it would not work.

I use crsf all time on my receivers, btw.

maybe you could check that config_crsf() is compiled in by simply adding a dbg.puts line

jlpoltrack commented 1 year ago

Thanks for the hints. Hmm, happens on E5 Mini as well. Rx.OutMode reports as '001' via dbg when calling after this line: https://github.com/olliw42/mLRS/blob/main/mLRS/CommonRx/mlrs-rx.cpp#L578

The issue seems to be with the stdstm32-uart.h module & LPUART. Note 'uart_setprotocol' is calling USART (not LPUART items): https://github.com/olliw42/stm32ll-lib/blob/b597d50526067a2b605be675b0214de9a0b7d2d9/src/stdstm32-uart.h#LL581-L593C2 so when 'uart_setprotocol' is called I don't think it does anything for the LPUART and just ends up using the initial structure https://github.com/olliw42/stm32ll-lib/blob/b597d50526067a2b605be675b0214de9a0b7d2d9/src/stdstm32-uart.h#L692

If I change baud in the HAL that flows through to what I see in the LPUART initial structure and matches what I see on the scope:

LP Baud

LPUART

I added another ifdef for STM32WL like this and CRSF is now 416666 baud even with 123456 set in the HAL:

STM32WL

Let me know if this makes sense and I can submit a PR. Edit - I don't think the LPUART stuff needs to be specific to STM32WL...

olliw42 commented 1 year ago

hmhmhm

the reason uart_setbaudrate() is supported in the lib for LPUART for STM32WL, but not in general /e.g. not for G4), is that at the time when I did this I concluded that for the STM32WL one can access the LPUART with the same LL functions as the "normal" uarts, since the LPUART LL functions also use the USART_TypeDef as pointer to teh uart (which is not so for e.g. G4). I believe I had tested this, so while I would not rule out I got this wrong I'm at least temporarily confused.

for uart_rx_enableisr() and many other operations in uart_init_isroff() this appears to work.

What you are saying is that one needs to use the LPUART LL functions in uart_setbaudrate(), right?

olliw42 commented 1 year ago

yes, it would not have to be specific to the WL, and could support also e.g. the G4 HOWEVER: It needs to be explicitely tested and confirmed for the specific mcu!! otherwise I would not accept it be included. (only explicitely tested cases I include)(and that's why I'm confused because if I would not have tested set_baudrate() for the WL I would not have included it)(but as said, I could have done something wrong)(but you get the idea :))

jlpoltrack commented 1 year ago

Just to confirm, you don't see this on your STM32WL boards when using LPUART?

I can't. All I can do is to say what I believe that I had tested this on a STM32WL board at the time I added this.

I only checked the 'uart_setprotocol' function which is called when the 'config_crsf' flag is set to true, didn't look at 'uart_setbaudrate' so can't comment there.

whatever holds true for the one should hold true for the other, uart_setbaudrate() is just a slimer version of uart_setprotocol(). Maybe one should actually change it to just call uart_setprotocol(). (and maybe also in init)

jlpoltrack commented 1 year ago

I did a little more testing and can confirm with dbg that the current code generates an error when it calls the 'LL_USART_Init' function for LPUART, function returns ErrorStatus = 0. When using the LL_LPUART_Init, I get ErrorStatus = 1.

You're definitely right in that most functions for LPUART and USART look interchangeable for STM32WL but init function has differences and I guess that's enough.

https://github.com/olliw42/mLRS/blob/main/mLRS/rx-Wio-E5-Mini-wle5jc/Drivers/STM32WLxx_HAL_Driver/Src/stm32wlxx_ll_lpuart.c#L168

https://github.com/olliw42/mLRS/blob/main/mLRS/rx-Wio-E5-Mini-wle5jc/Drivers/STM32WLxx_HAL_Driver/Src/stm32wlxx_ll_usart.c#L173

olliw42 commented 1 year ago

first, sorry I have mistakenly edited your previosu post ... for some reason I continue to sometimes step into this trap

2nd, ok, so it seems I must have done some nonsense here and setbaudrate doesn't work for the WL. You want to raise a PR? Otherwise I would correct.

thx for bringing this up.

jlpoltrack commented 1 year ago

All good. If you want to correct (not 100% sure of how you want to address) I can test out the change.

olliw42 commented 1 year ago

@jlpoltrack I have oushed some changes which hopefully resolve all these issues. MANY thx for your research and help in this matter!

closing this. Pl do not hesitate to reopen if you consider it appropriate.