Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Channel scan configuration #1190

Merged
merged 39 commits into from
Sep 1, 2024
Merged

Channel scan configuration #1190

merged 39 commits into from
Sep 1, 2024

Conversation

jgromes
Copy link
Owner

@jgromes jgromes commented Aug 18, 2024

Added channel scan configuration via PHY. Currently implemented for SX126x, SX128x and LR11x0.

@jgromes jgromes requested a review from StevenCellist August 18, 2024 09:11
Copy link
Collaborator

@StevenCellist StevenCellist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgromes thank you very much!! I think two points could use a slight change, but overall much appreciated and very useful :)

src/protocols/PhysicalLayer/PhysicalLayer.h Outdated Show resolved Hide resolved
src/modules/SX126x/SX126x.cpp Show resolved Hide resolved
@StevenCellist
Copy link
Collaborator

StevenCellist commented Aug 21, 2024

@jgromes thanks for the changes. I will test the CAD specifics later, and I am currently looking at PhysicalLayer::startReceive(uint32_t timeout, uint32_t irqFlags, uint32_t irqMask, size_t len). The irqFlags and irqMask methods now seem redundant, as these can now be set through setIrq() in a module-independent way. Do you agree on this?
The only catch I can think of is that module-specific IRQs are then lost, but I'm not sure if that hinders anything (or some can always be added later to the general IRQs).

EDIT: hmm, this is not as easy as I thought - would it be an idea to keep the IRQ parameters, but give them their new abstract form that is then resolved into the module-specific IRQs?

@StevenCellist StevenCellist self-requested a review August 21, 2024 21:28
Copy link
Collaborator

@StevenCellist StevenCellist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spent the past hour investigating the new things in this PR. The problem I encountered, is that it is not possible to configure multiple flags through the RadioIrqFlags_t type as it is now, because the IRQ flags are not unique bits, but rather indices. This will require some rethinking, as using bits then again doesn't work as a straight index into irqMap... hmm.

@StevenCellist
Copy link
Collaborator

Part of the problem is that most methods (re)configure the IRQs themselves. The new functions to set IRQs are all good, but overruled as soon as a method is called.
Sorry, just a bunch of thoughts here, not much structure.

@jgromes
Copy link
Owner Author

jgromes commented Aug 23, 2024

it is not possible to configure multiple flags through the RadioIrqFlags_t type as it is now

That should be fairly straight-forward, we can have PhysicalLayer::setIrq(uint32_t) which takes multiple bits, then iterates over them and calls the lower level for every enabled bit, passing its index.

most methods (re)configure the IRQs themselves

That is indeed a bit more tricky. I think we can leave it as it is now and revisit later, since CAD configures the flags on the module level, not on the PHY level. But at least these new abstractions allow us to get rid of the PhysicalLayer::irqRxDoneRxTimeout method.

@StevenCellist
Copy link
Collaborator

New IRQs passed the test through LoRaWAN's downlinkCommon() after one compilation error:

lib/RadioLib/src/modules/SX127x/SX127x.cpp: In member function 'virtual int16_t SX127x::startReceive()':
lib/RadioLib/src/modules/SX127x/SX127x.cpp:389:121: error: call of overloaded 'startReceive(int, int, long unsigned int, long unsigned int)' is ambiguous
   return(this->startReceive(0, RADIOLIB_SX127X_RXCONTINUOUS, RADIOLIB_IRQ_RX_DEFAULT_FLAGS, RADIOLIB_IRQ_RX_DEFAULT_MASK));
                                                                                                                         ^
In file included from lib/RadioLib/src/modules/SX127x/SX127x.cpp:1:
lib/RadioLib/src/modules/SX127x/SX127x.h:822:13: note: candidate: 'int16_t SX127x::startReceive(uint8_t, uint8_t, RadioLibIrqFlags_t, RadioLibIrqFlags_t)'
     int16_t startReceive(uint8_t len, uint8_t mode = RADIOLIB_SX127X_RXCONTINUOUS, RadioLibIrqFlags_t irqFlags = RADIOLIB_IRQ_RX_DEFAULT_FLAGS, RadioLibIrqFlags_t irqMask = RADIOLIB_IRQ_RX_DEFAULT_MASK);
             ^~~~~~~~~~~~
lib/RadioLib/src/modules/SX127x/SX127x.h:835:13: note: candidate: 'virtual int16_t SX127x::startReceive(uint32_t, RadioLibIrqFlags_t, RadioLibIrqFlags_t, size_t)'
     int16_t startReceive(uint32_t timeout, RadioLibIrqFlags_t irqFlags, RadioLibIrqFlags_t irqMask, size_t len) override;
             ^~~~~~~~~~~~
*** [.pio\build\esp32-heltec-v3\lib5ef\RadioLib\modules\SX127x\SX127x.cpp.o] Error 1

Specifically casting the mode argument to uint8_t resolved the problem in this case.

@StevenCellist
Copy link
Collaborator

StevenCellist commented Aug 25, 2024

And the actual CAD is working all good now, too :)
Here's a snippet I use right now which is still partially module-dependent (the 0xFF values translate to CAD_PARAM_DEFAULT, 0x01 is for CadToRx - both of these technically work for all modules that accept these values, though).

  ChannelScanConfig_t cadCfg;
  cadCfg.cad.symNum = 0xFF;
  cadCfg.cad.detPeak = 0xFF;
  cadCfg.cad.detMin = 0xFF;
  cadCfg.cad.exitMode = 0x01;
  cadCfg.cad.irqFlags  = 0;
  cadCfg.cad.irqFlags |= (1UL << RADIOLIB_IRQ_CAD_DONE);
  cadCfg.cad.irqFlags |= (1UL << RADIOLIB_IRQ_CAD_DETECTED);
  cadCfg.cad.irqFlags |= (1UL << RADIOLIB_IRQ_PREAMBLE_DETECTED);
  cadCfg.cad.irqFlags |= (1UL << RADIOLIB_IRQ_HEADER_VALID);
  cadCfg.cad.irqFlags |= (1UL << RADIOLIB_IRQ_RX_DONE);
  cadCfg.cad.irqFlags |= (1UL << RADIOLIB_IRQ_TIMEOUT);
  cadCfg.cad.irqMask = cadCfg.cad.irqMask;
  state = this->phyLayer->startChannelScan(cadCfg);

Depending on how we would want to resolve this, all looks very good to me - thank you very much!!

@StevenCellist StevenCellist self-requested a review August 25, 2024 21:24
@jgromes
Copy link
Owner Author

jgromes commented Aug 29, 2024

@StevenCellist

New IRQs passed the test through LoRaWAN's downlinkCommon() after one compilation error:

Ah, the error was caused by the old SX127x start receive method, which exposed the Rx mode directly to user. That no longer makes sense, so the Rx mode is now deduced based on timeout - fixed now.

FYI because if the changes to the channel scanning and receive methods, I will keep this open until I can run some basic checks on a few devices, hopefully in the next few days.

@jgromes jgromes merged commit bc801c7 into master Sep 1, 2024
30 checks passed
@jgromes
Copy link
Owner Author

jgromes commented Sep 1, 2024

@StevenCellist @GUVWAF I was able to test this on SX1278, SX1262 and LR1110, all seems to be working (as in: transmission, reception and CAD). Hopefully I didn't miss something :)

@jgromes jgromes deleted the scan-config branch September 1, 2024 16:20
@StevenCellist
Copy link
Collaborator

Thank you very much!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants