LoRa device type + SX1262 kernel driver (first checkpoint)#565
LoRa device type + SX1262 kernel driver (first checkpoint)#565Crazypedia wants to merge 5 commits into
Conversation
Add a LORA_TYPE kernel device type and LoraApi for sub-GHz packet transceivers: enable/disable, modulation selection (LoRa / FSK / LR-FHSS), float-valued per-modulation parameters with documented units, a copy-in TX queue with per-packet progress, and RX/TX/state callbacks. Modulation-agnostic with can_transmit / can_receive capability queries so callers can discover what a given modem supports. Sits alongside the wifi and bluetooth device types (function-specific, not a generic "radio"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implement the lora device type for the Semtech SX1262 on top of RadioLib (MIT, managed component jgromes/radiolib). Registers a lora device as a child of an SPI controller; CS comes from the parent's cs-gpios entry. RadioLib is kept behind a RadioParts pimpl so its global `class Module` never shares a translation unit with the kernel's `struct Module`. - Runs the radio on a dedicated thread with an event-group mailbox; callbacks fire without the driver lock held (snapshot + release) so consumers can take their own locks in callbacks without an AB-BA deadlock. - DIO1 uses the kernel GPIO descriptor callback API in HIGH_LEVEL mode, armed per cycle and masked by the ISR on each fire. Edge interrupts are avoided on purpose (ESP32 erratum 3.11: a subsequent edge interrupt may be missed, which for a radio would drop an RX/TX-done and stall the thread). - SPI exchanges run inside the kernel SPI controller lock (outer) and ESP-IDF's per-host bus lock (inner), so a manually-CS'd RadioLib exchange is atomic against both cooperating kernel drivers and the IDF-managed display/SD devices. - start_device selects the antenna path, powers the module and runs a GPIO-only probe (reset pulse -> wait for BUSY low) so an absent/miswired module fails at device start; the modem is configured only when the radio is enabled. - CS/RESET/BUSY/DIO1 must be SoC GPIOs (the RadioLib HAL drives them directly); the optional enable and antenna-select lines may sit behind an IO expander. - TX-done timeout is derived from time-on-air; PA over-current protection defaults to RadioLib's fail-safe 60 mA and is raisable via LORA_PARAMETER_CURRENT_LIMIT. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the SX1262 radio node to the T-Deck Max devicetree (CS on SPI index 2 / GPIO3, reset/DIO1/busy on GPIO 4/5/6, enable and antenna-select on the xl9555 expander, 2.4 V TCXO, DIO2 as RF switch) and declare the sx126x-module dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a public LoRa driver API with modulation, parameter, transmission, reception, and callback interfaces. Introduces an SX1262 module with devicetree bindings, RadioLib integration, ESP-IDF GPIO/SPI handling, threaded radio operations, and driver lifecycle wiring. Registers the API symbols and RadioLib dependency, then configures SX1262 support and board-specific GPIO wiring for the LilyGO T-Deck Max. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Devices/lilygo-tdeck-max/lilygo,tdeck-max.dts (1)
64-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the
regproperty to match the unit address.The node is named
radio@2, but lacks the correspondingreg = <2>;property. Standard device tree compilers typically emit a warning when a node has a unit name but noregproperty. While the system appears to correctly infer the SPI chip-select index from thecs-gpiosarray, addingregexplicitly aligns with standard devicetree conventions.♻️ Proposed refactor
radio@2 { compatible = "semtech,sx1262"; + reg = <2>; pin-reset = <&gpio0 4 GPIO_FLAG_NONE>; pin-dio1 = <&gpio0 5 GPIO_FLAG_NONE>;Drivers/sx126x-module/private/drivers/sx126x_radiolib_hal.cpp (1)
23-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStop reading
gpiohal.dev->pin[pin].int_typedirectly. ESP-IDF doesn’t expose a public getter for the current interrupt type, so this helper should track the intendedintr_typeitself (or document the private HAL dependency) instead of relying onGPIO_LL_GET_HW/gpio_hal_context_tinternals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b3c4dd1-adf2-494c-ba99-4a66295768cc
📒 Files selected for processing (20)
Devices/lilygo-tdeck-max/devicetree.yamlDevices/lilygo-tdeck-max/lilygo,tdeck-max.dtsDrivers/sx126x-module/CMakeLists.txtDrivers/sx126x-module/LICENSE-Apache-2.0.mdDrivers/sx126x-module/README.mdDrivers/sx126x-module/bindings/semtech,sx1262.yamlDrivers/sx126x-module/devicetree.yamlDrivers/sx126x-module/include/bindings/sx1262.hDrivers/sx126x-module/include/drivers/sx1262.hDrivers/sx126x-module/include/sx126x_module.hDrivers/sx126x-module/private/drivers/sx1262_radio.cppDrivers/sx126x-module/private/drivers/sx1262_radio.hDrivers/sx126x-module/private/drivers/sx126x_radiolib_hal.cppDrivers/sx126x-module/private/drivers/sx126x_radiolib_hal.hDrivers/sx126x-module/source/module.cppDrivers/sx126x-module/source/sx1262.cppFirmware/idf_component.ymlTactilityKernel/include/tactility/drivers/lora.hTactilityKernel/source/drivers/lora.cppTactilityKernel/source/kernel_symbols.c
- Service a received packet before deciding to transmit: an RX-done and a queued TX could coincide in the same radio-thread iteration, and the old else-branch dropped the packet outright. - Publish a terminal ERROR state when the radio thread is interrupted after finishTransmit(), so a caller whose TX raced setEnabled(false) still gets a final callback for its id. - Deactivate pin-antenna-select on probe failure too, mirroring stop(). - Add the reg property for the radio's SPI unit address: declared in the binding (the devicetree compiler rejects undeclared properties), carried in Sx1262Config, and checked against the node's unit address at device start so the two can't silently disagree. - Drop the private gpio_hal_context_t read in the RadioLib HAL's pinMode(): configure pad routing, direction and pulls through public per-aspect APIs that leave the interrupt configuration untouched, so the kernel-owned DIO1 HIGH_LEVEL interrupt survives without reading ESP-IDF HAL internals. Hardware-validated on the LilyGO T-Deck Max: GPIO probe, modem init through the new pinMode path, TX-done at real airtime, and RX from three nodes interleaved with TX, with no errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both nitpicks addressed in 87076e1, each with a wrinkle worth noting:
Private |
|
Thank you! I will look at this in detail in the coming days, but the basic premise looks good. |
# Conflicts: # TactilityKernel/source/kernel_symbols.c
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
Looks good! I only have 1 area of feedback with regards to using float values.
| LORA_PARAMETER_FREQUENCY_DEVIATION, | ||
| LORA_PARAMETER_DATA_RATE, | ||
| LORA_PARAMETER_NARROW_GRID, | ||
| LORA_PARAMETER_CURRENT_LIMIT, |
There was a problem hiding this comment.
Some things to change:
- The parameter documentation should be above each enum value. It makes it easier to see if docs are missing.
LoraParameterseems to work with floats, but float is not a reliable format: If you have a value like1.1fit might internally be something like1.09999999instead. Let's make it aint32_tinstead.SYNC_WORD: doesn't mention what the possible values are.
There was a problem hiding this comment.
I'd also like to see the private/drivers/sx1262_radio.cpp adapter to use int32_t instead of floats.
|
Sorry about the git conflicts 😅 |
|
No worries I'll resolve it when I do the hardware checks tomorrow |
2 participants
Tracking: #342
Adds sub-GHz radio support to Tactility as a first-class kernel device type, plus a
driver for the Semtech SX1262. Continues the radio work discussed in #342, brought up to
the current kernel driver model (rather than the deprecated
tt::hallayer the earlierprototype targeted).
What this adds
loradevice type (<tactility/drivers/lora.h>,TactilityKernel): a C ABI forsub-GHz packet transceivers — enable/disable, modulation selection, per-modulation
parameters (float-valued, documented units), a copy-in TX queue with per-packet
progress, and RX/TX/state callbacks. Modulation-agnostic by design (LoRa, FSK,
LR-FHSS), with
can_transmit/can_receivecapability queries so callers can discoverwhat a given modem supports.
sx126x-modulekernel driver: implements theloraAPI for the SX1262 on top ofRadioLib (managed component
jgromes/radiolib,pinned). Registers a
loradevice as a child of an SPI controller; CS comes from theparent's
cs-gpiosentry.semtech,sx1262) + example, and wiring on the LilyGOT-Deck Max.
Design notes for review
class Modulethat clasheswith the kernel's
struct Module. RadioLib headers are kept out of every header via aRadioPartspimpl, so no translation unit sees both.must be atomic on the bus. It runs inside two nested locks — the kernel SPI controller
lock (
spi_controller_lock, the arbiter other kernel drivers on the host cooperatethrough) as the outer lock, and ESP-IDF's per-host bus lock (
spi_device_acquire_bus)as the inner one, which additionally blocks the IDF-managed spi_master devices (EPD
display, SD) that don't take the controller lock. Nothing else takes both, so there is
no lock-ordering hazard.
HIGH_LEVEL mode. Edge-triggered interrupts are avoided on purpose (ESP32 erratum
3.11: a subsequent edge interrupt may be missed, which for a radio would drop an
RX/TX-done and stall the thread). The radio thread arms the interrupt each cycle; the
ISR masks it on each fire and signals the thread, which re-arms after clearing the modem
IRQ. RadioLib's
attachInterrupt/detachInterruptare deliberately no-ops.start_deviceselects the antenna path, powers the module, andruns a GPIO-only probe (reset pulse → wait for BUSY low) so an absent/unpowered or
miswired module fails at device start instead of as an opaque RadioLib error later. The
modem is only configured when the radio is enabled via
lora_set_enabled()(requires amodulation first). Teardown stops the radio thread, sleeps the modem, and powers down in
reverse order.
RadioLib's HAL drives them directly through ESP-IDF, so they can't sit behind an IO
expander. The driver-registration layer enforces this (
acquire_native_pinfails with"must be an SoC GPIO"). The optional enable and antenna-select lines may sit behind an
IO expander (e.g. xl9555 on the T-Deck Max), resolved through the GPIO descriptor API
with active-low polarity applied by the driver. A board that routes reset/CS through an
expander is therefore not supported by this first checkpoint; the constraint is a
deliberate, documented scope limit rather than an accident, and the kernel
loraAPI isunaffected either way.
getTimeOnAir) plus a fixedmargin, so slow configs (high SF / narrow BW, airtime up to seconds) don't
false-time-out; a fixed fallback covers configs RadioLib can't compute airtime for.
caps PA current into a bad/disconnected antenna but also caps output below +22 dBm.
Because Tactility targets a variety of hardware (not all T-Decks), the conservative
default is kept and a board-aware consumer can opt into a higher limit via
LORA_PARAMETER_CURRENT_LIMIT(up to 140 mA). OCP is not auto-derived from output power.for RX/TX-progress/state, or the caller's thread for the QUEUED event), so a consumer
can safely take its own locks in a callback.
Hardware validation
OK, modem init, TX-done at the expected airtime, clean teardown to sleep, no EPD/SD bus
clashes on the shared host.
messages, exchanged against other nodes) — the RX path was exercised through an
out-of-tree consumer that sits on this same
loraAPI; that consumer is not part of thisPR, but it proves the driver's RX/TX-done duty-cycle and callback delivery on hardware.
POSIX/simulator test suites, so the driver is self-contained.
Known limitations / scope
sx126xfamily to leave room forSX1261/SX1268). RadioLib supports many other transceivers, so additional chips can be
added as sibling driver modules implementing the same
loraAPI, with no kernel change.above the driver.
loraAPI.antenna-detect pin (unlike, e.g., u-blox SARA modules' dedicated ANT_DET ADC circuit),
and the T-Deck Max connector is not hot-swap-rated. The intended approach is a software
heuristic on the radio thread — watching the RX noise floor / RSSI for the signature of
a disconnected/open PA load and disabling TX at the driver layer when detected, so the
protection lives below the app/OS. This is unproven; noting the attempt and intended
method here to invite feedback on the method before building it.
loradevice-type dispatch layer builds on POSIXbut the SX1262 driver is ESP32-only (RadioLib + ESP GPIO/SPI).
Status
First checkpoint. The radio is functional and HW-validated; the items under "known
limitations" are the acknowledged next steps rather than blockers. Opening against #342 as
an in-progress milestone.
Files
Three commits, off the current
main:feat(kernel): add generic LoRa device type API—TactilityKernellora.h/lora.cpp/kernel_symbols.cfeat(drivers): add sx126x-module kernel driver for the Semtech SX1262—Drivers/sx126x-module/(driver, RadioLib HAL shim, binding, README) +Firmware/idf_component.yml(RadioLib dependency)feat(lilygo-tdeck-max): wire up the SX1262 LoRa radio—Devices/lilygo-tdeck-max/devicetree wiring
🤖 Generated with Claude Code
Summary by CodeRabbit