search

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
266 stars 55 forks source link

common, fix antenna 2 on single spi #202

Closed jlpoltrack closed 1 week ago

jlpoltrack commented 2 weeks ago

To address: https://github.com/olliw42/mLRS/issues/200

Tested on SuperD

olliw42 commented 2 weeks ago

THX!

I unfortunately don't like it all however. This breaks all abstraction, layering & architecture. The simplest approach to disentangle would be let Init have a parameter, like bool initialize_spi, and set that properly upon call in main. Structure-wise it's all so nice, but I don't have any good idea either :(

olliw42 commented 2 weeks ago

it is actually also surprising it works

first, Config is populated in setup_init(); which is called only after sx2.Init(); ....

https://github.com/olliw42/mLRS/blob/main/mLRS/CommonTx/mlrs-tx.cpp#L274-L277 https://github.com/olliw42/mLRS/blob/main/mLRS/CommonRx/mlrs-rx.cpp#L147-L150

moreover, since sx.Init() is called anyway, whatever antenna is selected, spi should have been inited already, so the additional code in sx2.Init() is just doing what was done already before.

jlpoltrack commented 1 week ago

it is actually also surprising it works

It doesn't - the ESP soft reboot after parameter save made it looked like it did. Going to close.

olliw42 commented 1 week ago

It doesn't - the ESP soft reboot after parameter save made it looked like it did.

ha, yeah, this works ... the crash is after repowering

good to see we have at least some consistency

MANY THX anyways

jlpoltrack commented 1 week ago

One last thought - for the time being should we change the diversity mask for single SPI so that it doesn't allow Antenna 2 to be selected? Currently if you select Antenna 2 you'd get stuck and would have to wipe / reflash: https://github.com/olliw42/mLRS/blob/main/mLRS/Common/setup.h#L87-L89

olliw42 commented 1 week ago

I had that thought too but kind of was hoping that the proper solution might found in not too distant future ... but if we don't I guess we should do exactly that, at least as momentry precaution.