Add SX1262 LoRa driver and sample#1516
Open
garydyksman wants to merge 20 commits into
Open
Conversation
- Add Sx1262 Driver - Add Sx1262Sample app
Increase buffer size by one byte and skip three protocol bytes (instead of two) when reading from the RX buffer to properly handle the required NOP and return only the payload.
nfbot
added
Type: bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Member
|
@garydyksman thanks a lot for this nice addition. Before going for a deeper code review, can you please add StyleCop? See https://github.com/nanoframework/nanoFramework.IoT.Device/tree/main/StyleCop A lot of things will be catch up by it. I4ll then do a proper review. |
Member
|
Also you are missing nuget and few other things. Check the other devices. |
Member
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds a new nanoFramework LoRa device binding under devices/LoRa/ with a common ILoRaDevice abstraction, an SPI-based SX1262 implementation, and a runnable sample to validate TX/RX behavior on hardware.
Changes:
- Introduces
Iot.Device.LoRalibrary project withILoRaDevice+Sx1262driver implementation. - Adds
Sx1262Sampleapp demonstrating initialization, transmit loop, and receive polling. - Adds solution/project/package configuration files to build the library and sample.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| devices/LoRa/LoRa.sln | New solution wiring the LoRa library and SX1262 sample. |
| devices/LoRa/LoRa/LoRa.nfproj | New library project definition for Iot.Device.LoRa. |
| devices/LoRa/LoRa/packages.config | NuGet package references for the LoRa library project. |
| devices/LoRa/LoRa/Properties/AssemblyInfo.cs | Library assembly metadata (title/company/copyright). |
| devices/LoRa/LoRa/ILoRaDevice.cs | Introduces shared LoRa abstractions (ILoRaDevice, message/event types). |
| devices/LoRa/LoRa/Drivers/Sx1262.cs | SX1262 driver implementation (SPI + GPIO, TX/RX, polling, buffer reads). |
| devices/LoRa/Sx1262Sample/Sx1262Sample.nfproj | Sample project definition referencing the LoRa library. |
| devices/LoRa/Sx1262Sample/packages.config | NuGet package references for the sample project. |
| devices/LoRa/Sx1262Sample/Properties/AssemblyInfo.cs | Sample assembly metadata. |
| devices/LoRa/Sx1262Sample/Program.cs | Sample app main loop and RX event handler. |
Wire StyleCop.MSBuild and Settings.StyleCop for the library; add Nerdbank.GitVersioning, signing, packages.lock.json, version.json, nuspec, category, and README like other bindings. Split LoRaMessage and PacketReceivedHandler for file/type naming rules; fix XML docs and layout (including SA1514) in Sx1262, ILoRaDevice, and the sample; remove StyleCop from the sample to match At24cxx-style samples. Made-with: Cursor
- Use Initialize spelling on ILoRaDevice, Sx1262, and sample. - Avoid Join deadlock when StopPolling runs on the poll thread; clear poll thread in finally. - Add LoRaTests project with DecodeChipMode unit tests; wire solution and README. Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devices/LoRa/LoRa.nuspec`:
- Line 18: Update the <description> element in LoRa.nuspec to use nanoFramework
wording rather than ".NET IoT Core binding"; specifically replace the phrase
"the .NET IoT Core binding Iot.Device.LoRa" with wording like "the nanoFramework
binding Iot.Device.LoRa" (so the description accurately states the package
targets .NET nanoFramework C# projects).
In `@devices/LoRa/LoRa/Drivers/Sx1262.cs`:
- Around line 385-418: The HandleRxDone method currently clears IRQs then
returns early on timeout, CRC error, or non-RxDone conditions without restarting
RX; ensure RX is always restarted after ClearIrqStatus by invoking
StartReceiving() before any early return (or wrap the IRQ handling in a
try/finally that calls StartReceiving()), so that for all branches in
HandleRxDone (when checking IrqTimeout, IrqCrcErr, and IrqRxDone)
StartReceiving() is executed prior to returning; update the early-return paths
in HandleRxDone to call StartReceiving() (or use a finally) and keep existing
calls to GetRxBufferStatus, ReadBuffer, GetPacketStatus, PacketReceived, and the
final return unchanged.
- Around line 273-316: Before changing radio state, record whether polling was
active (e.g., bool wasPolling = /* check current poll/thread state from
StartPolling/StopPolling usage or pollingThread != null */), then call
StopPolling(); after stopping, clear any stale IRQs by calling
ClearIrqStatus(0xFFFF) (or ClearIrqStatus(GetIrqStatus())) before writing the TX
command so a pre-existing DIO1 won't be mistaken for the new TxDone; proceed to
WriteCommand(OpSetTx, ...) and wait for IsDio1High as before, then read irq =
GetIrqStatus() and handle IrqTimeout/IrqTxDone; finally, only call
StartPolling() in the finally block if wasPolling was true so you don't start
polling for callers that never started it.
- Line 60: Current ownership flag _shouldDispose is being used for both GPIO and
SPI, but Dispose() only disposes gpioController and never calls _spi.Dispose(),
leaving the SPI handle open; change ownership so _shouldDispose continues to
control only gpioController (set using gpioController == null logic in the
constructor) and ensure _spi is always disposed inside Dispose(): call
_spi.Dispose() (or null-check and dispose) unconditionally when disposing the
driver, while retaining conditional disposal for gpioController using
_shouldDispose; update the constructor/comment if needed to clarify that
_shouldDispose pertains to GPIO only.
In `@devices/LoRa/LoRa/LoRaMessage.cs`:
- Around line 32-37: The LoRaMessage constructor should validate and defensively
copy the payload: in the LoRaMessage(byte[] payload, int rssi, float snr)
constructor, throw an ArgumentNullException if payload is null (unless null is
intended and documented) and assign a defensive copy (e.g., create a new byte[]
and copy or use payload.ToArray()) to the read-only Payload property to prevent
external mutation; keep Rssi and Snr assignments unchanged.
In `@devices/LoRa/LoRa/Properties/AssemblyInfo.cs`:
- Line 8: Update the assembly attribute value for assembly: AssemblyCopyright to
use the standard "Copyright (c)" formatting by inserting a space between
"Copyright" and "(c)"; locate the AssemblyCopyright attribute in AssemblyInfo.cs
and change the string "Copyright(c).NET Foundation and Contributors" to
"Copyright (c).NET Foundation and Contributors".
In `@devices/LoRa/LoraTests/nano.runsettings`:
- Line 15: Fix the typo in the XML comment associated with
PathToLocalCLRInstance by replacing "witll" with "will" so the comment reads
"...default one installed with nanoclr CLR will be used."; update the comment
text next to the PathToLocalCLRInstance element accordingly.
In `@devices/LoRa/Sx1262Sample/Properties/AssemblyInfo.cs`:
- Line 8: Update the assembly attribute value in AssemblyInfo.cs so the
copyright string uses the standard spacing: change the AssemblyCopyright
attribute value from "Copyright(c).NET Foundation and Contributors" to include a
space after "Copyright" (i.e., "Copyright (c) ..."); edit the AssemblyCopyright
declaration to reflect this corrected string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5421710-8b16-43cd-a165-d575ef6a8110
📒 Files selected for processing (24)
devices/LoRa/LoRa.nuspecdevices/LoRa/LoRa.slndevices/LoRa/LoRa/Drivers/Sx1262.csdevices/LoRa/LoRa/ILoRaDevice.csdevices/LoRa/LoRa/LoRa.nfprojdevices/LoRa/LoRa/LoRaMessage.csdevices/LoRa/LoRa/PacketReceivedHandler.csdevices/LoRa/LoRa/Properties/AssemblyInfo.csdevices/LoRa/LoRa/Settings.StyleCopdevices/LoRa/LoRa/packages.configdevices/LoRa/LoRa/packages.lock.jsondevices/LoRa/LoraTests/LoRaTests.nfprojdevices/LoRa/LoraTests/Properties/AssemblyInfo.csdevices/LoRa/LoraTests/Sx1262DecodeChipModeTests.csdevices/LoRa/LoraTests/nano.runsettingsdevices/LoRa/LoraTests/packages.configdevices/LoRa/README.mddevices/LoRa/Sx1262Sample/Program.csdevices/LoRa/Sx1262Sample/Properties/AssemblyInfo.csdevices/LoRa/Sx1262Sample/Sx1262Sample.nfprojdevices/LoRa/Sx1262Sample/packages.configdevices/LoRa/Sx1262Sample/packages.lock.jsondevices/LoRa/category.txtdevices/LoRa/version.json
There was a problem hiding this comment.
♻️ Duplicate comments (2)
devices/LoRa/LoRa/Drivers/Sx1262.cs (2)
397-426:⚠️ Potential issue | 🟠 MajorRestart RX in a
finallyso read failures do not stop reception.After Line 400 clears IRQs, exceptions from
GetRxBufferStatus,ReadBuffer,GetPacketStatus, orLoRaMessageconstruction skip Line 426. The poll loop then logs the error, but the radio is not put back into RX mode, so future packets can be missed.Proposed fix
public LoRaMessage HandleRxDone() { ushort irq = GetIrqStatus(); ClearIrqStatus(0xFFFF); - if ((irq & IrqTimeout) != 0) - { - StartReceiving(); - return null; - } - - if ((irq & IrqCrcErr) != 0) - { - StartReceiving(); - return null; - } - - if ((irq & IrqRxDone) == 0) - { - StartReceiving(); - return null; - } - - GetRxBufferStatus(out byte length, out byte offset); - byte[] payload = ReadBuffer(offset, length); - - GetPacketStatus(out int rssi, out float snr); - LoRaMessage msg = new LoRaMessage(payload, rssi, snr); - - StartReceiving(); + LoRaMessage msg = null; + try + { + if ((irq & IrqTimeout) != 0) + { + return null; + } + + if ((irq & IrqCrcErr) != 0) + { + return null; + } + + if ((irq & IrqRxDone) == 0) + { + return null; + } + + GetRxBufferStatus(out byte length, out byte offset); + byte[] payload = ReadBuffer(offset, length); + + GetPacketStatus(out int rssi, out float snr); + msg = new LoRaMessage(payload, rssi, snr); + } + finally + { + StartReceiving(); + } if (PacketReceived != null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devices/LoRa/LoRa/Drivers/Sx1262.cs` around lines 397 - 426, The HandleRxDone method currently calls StartReceiving() only on the success path so exceptions from GetRxBufferStatus, ReadBuffer, GetPacketStatus, or the LoRaMessage constructor prevent the radio from being restarted; refactor HandleRxDone to ensure StartReceiving() is invoked in a finally block (after clearing IRQs) so that regardless of exceptions the radio is returned to RX, keeping the calls to GetRxBufferStatus, ReadBuffer, GetPacketStatus and LoRaMessage construction unchanged but wrapped in try/catch as needed and rethrow or handle errors while guaranteeing StartReceiving() runs.
57-59:⚠️ Potential issue | 🟠 MajorDispose the SPI device when the driver is disposed.
Sx1262keeps the acceptedSpiDevicefor its lifetime, butDispose()only releases GPIO resources. This leaves the SPI handle open after driver disposal; keep GPIO ownership conditional, but release_spias part of the driver teardown.Proposed fix
public void Dispose() { if (_disposed) { return; @@ if (_shouldDispose && _gpio != null) { _gpio.Dispose(); } + _spi.Dispose(); + _disposed = true; }Also applies to: 513-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devices/LoRa/LoRa/Drivers/Sx1262.cs` around lines 57 - 59, The Sx1262 Dispose method currently only releases GPIO; modify the teardown so the SpiDevice (_spi) is disposed as part of driver disposal and GPIO (_gpio) remains conditional on _shouldDispose. In the Sx1262.Dispose (or Dispose(bool) overload) call _spi.Dispose() (and set _spi = null) when disposing, and keep the existing conditional logic for _gpio using _shouldDispose; ensure the dispose pattern is followed (guard against multiple disposals) and update any constructor/ownership comments if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@devices/LoRa/LoRa/Drivers/Sx1262.cs`:
- Around line 397-426: The HandleRxDone method currently calls StartReceiving()
only on the success path so exceptions from GetRxBufferStatus, ReadBuffer,
GetPacketStatus, or the LoRaMessage constructor prevent the radio from being
restarted; refactor HandleRxDone to ensure StartReceiving() is invoked in a
finally block (after clearing IRQs) so that regardless of exceptions the radio
is returned to RX, keeping the calls to GetRxBufferStatus, ReadBuffer,
GetPacketStatus and LoRaMessage construction unchanged but wrapped in try/catch
as needed and rethrow or handle errors while guaranteeing StartReceiving() runs.
- Around line 57-59: The Sx1262 Dispose method currently only releases GPIO;
modify the teardown so the SpiDevice (_spi) is disposed as part of driver
disposal and GPIO (_gpio) remains conditional on _shouldDispose. In the
Sx1262.Dispose (or Dispose(bool) overload) call _spi.Dispose() (and set _spi =
null) when disposing, and keep the existing conditional logic for _gpio using
_shouldDispose; ensure the dispose pattern is followed (guard against multiple
disposals) and update any constructor/ownership comments if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a2617b3-f3f9-4b01-ac90-fae397b93c6f
📒 Files selected for processing (9)
devices/LoRa/LoRa.nuspecdevices/LoRa/LoRa/Drivers/Sx1262.csdevices/LoRa/LoRa/ILoRaDevice.csdevices/LoRa/LoRa/LoRaMessage.csdevices/LoRa/LoRa/Properties/AssemblyInfo.csdevices/LoRa/LoraTests/nano.runsettingsdevices/LoRa/README.mddevices/LoRa/Sx1262Sample/Program.csdevices/LoRa/Sx1262Sample/Properties/AssemblyInfo.cs
- Use standard .NET Foundation MIT file headers and AssemblyInfo preambles. - Send: ArgumentNullException for null payload; param names on ArgumentException. - Sx1262: add disposeSpi (default false) to dispose SpiDevice when owned by caller. - Clarify PacketReceivedHandler XML for EventHandler<T> limitation. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devices/LoRa/LoRa/Drivers/Sx1262.cs`:
- Around line 71-72: The poll thread stop flag and thread reference must be
synchronized: make _stopPolling (or access to it) use proper memory
synchronization and guard mutations/reads of _pollThread and _stopPolling with a
dedicated lock (e.g. a private readonly object _pollLock) so StartPolling(),
StopPolling(), Send(), and PollLoop() cannot race; acquire the lock when
setting/clearing _pollThread, setting _stopPolling, checking _pollThread before
calling StopPolling(), and when performing the sequence in Send() that may
interleave with polling, then signal the poll thread (set flag) and call Join()
while holding the appropriate synchronization to ensure the worker observes the
stop and the thread reference cannot be replaced concurrently.
- Around line 263-338: Serialize concurrent Send() calls by adding an operation
lock around the entire TX sequence: acquire a private lock object (e.g. _txLock
or reuse an existing operation lock) at the start of Send() before
ClearIrqStatus and release it in a finally block so the sequence from
ClearIrqStatus, WriteCommand(...OpSetPacketParams...), WriteBuffer(...),
WriteCommand(...OpSetTx...), the DIO1 wait loop (IsDio1High), GetIrqStatus and
the IRQ checks are executed atomically; keep the existing restartPolling logic
(StopPolling()/StartPolling()) and ensure StartPolling() is called in the outer
finally after releasing any per-send resources if you use Monitor.Enter/Exit or
lock() to avoid deadlocks with _pollThread/PacketReceived.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 582268e0-2f77-45c9-ae43-4590a0b5c2be
📒 Files selected for processing (7)
devices/LoRa/LoRa/Drivers/Sx1262.csdevices/LoRa/LoRa/ILoRaDevice.csdevices/LoRa/LoRa/LoRaMessage.csdevices/LoRa/LoRa/PacketReceivedHandler.csdevices/LoRa/LoRa/Properties/AssemblyInfo.csdevices/LoRa/LoraTests/Properties/AssemblyInfo.csdevices/LoRa/Sx1262Sample/Properties/AssemblyInfo.cs
- Move Sx1262Sample to samples/ and tests to tests/ per repo tooling conventions. - Move Settings.StyleCop to device folder; point StyleCopOverrideSettingsFile from LoRa.nfproj. - Update solution nesting, README, and sample package paths. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
devices/LoRa/LoRa/Drivers/Sx1262.cs (2)
473-506:⚠️ Potential issue | 🟠 MajorSynchronize
_pollThreadtransitions.
_pollThreadis still read/written from multiple threads without a common lock, so concurrentStartPolling(),StopPolling(),SendCore(), andPollLoop()can observe stale state or lose a start/stop transition.🔒 Suggested direction
private readonly object _sendLock = new object(); +private readonly object _pollLock = new object(); public void StartPolling() { - if (_pollThread != null) + lock (_pollLock) { - return; - } + if (_pollThread != null) + { + return; + } - Interlocked.Exchange(ref _stopPolling, 0); - StartReceiving(); + Interlocked.Exchange(ref _stopPolling, 0); + StartReceiving(); - _pollThread = new Thread(PollLoop); - _pollThread.Start(); + _pollThread = new Thread(PollLoop); + _pollThread.Start(); + } } public void StopPolling() { - Interlocked.Exchange(ref _stopPolling, 1); - Thread worker = _pollThread; + Thread worker; + lock (_pollLock) + { + Interlocked.Exchange(ref _stopPolling, 1); + worker = _pollThread; + } + if (worker == null) { return; @@ - if (Thread.CurrentThread != worker && _pollThread == worker) + lock (_pollLock) { - _pollThread = null; + if (Thread.CurrentThread != worker && _pollThread == worker) + { + _pollThread = null; + } } }Also applies to: 578-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devices/LoRa/LoRa/Drivers/Sx1262.cs` around lines 473 - 506, Access to _pollThread and transitions between StartPolling/StopPolling/PollLoop are unsynchronized; add a private readonly object (e.g., _pollLock) and use lock(_pollLock) around all reads/writes of _pollThread and any start/stop transitions (StartPolling, StopPolling, PollLoop and SendCore where _pollThread is checked) so thread A cannot observe stale or racy state. Keep using Interlocked/Volatile for _stopPolling (or use Volatile.Read/Write) but perform assignments and null-checks of _pollThread inside the same lock and set _pollThread = null while holding the lock; apply the same locking pattern to the other occurrence noted (the region around the other 578-607 methods).
422-451:⚠️ Potential issue | 🟠 MajorRestart RX from
finallyso read/status failures do not stop reception.After Line 425 clears IRQs, exceptions from
GetRxBufferStatus,ReadBuffer, orGetPacketStatusskip Line 451 and leave the radio out of RX mode;PollLooponly logs the exception, so receiving can silently stall.🐛 Proposed fix
public LoRaMessage HandleRxDone() { ushort irq = GetIrqStatus(); ClearIrqStatus(0xFFFF); + LoRaMessage msg = null; - if ((irq & IrqTimeout) != 0) + try { - StartReceiving(); - return null; - } + if ((irq & IrqTimeout) != 0) + { + return null; + } - if ((irq & IrqCrcErr) != 0) - { - StartReceiving(); - return null; - } + if ((irq & IrqCrcErr) != 0) + { + return null; + } - if ((irq & IrqRxDone) == 0) - { - StartReceiving(); - return null; - } + if ((irq & IrqRxDone) == 0) + { + return null; + } - GetRxBufferStatus(out byte length, out byte offset); - byte[] payload = ReadBuffer(offset, length); + GetRxBufferStatus(out byte length, out byte offset); + byte[] payload = ReadBuffer(offset, length); - GetPacketStatus(out int rssi, out float snr); - LoRaMessage msg = new LoRaMessage(payload, rssi, snr); - - StartReceiving(); + GetPacketStatus(out int rssi, out float snr); + msg = new LoRaMessage(payload, rssi, snr); + } + finally + { + StartReceiving(); + } if (PacketReceived != null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devices/LoRa/LoRa/Drivers/Sx1262.cs` around lines 422 - 451, The HandleRxDone method can throw from GetRxBufferStatus, ReadBuffer, or GetPacketStatus and currently StartReceiving is skipped, leaving the radio out of RX; wrap the block that calls GetRxBufferStatus, ReadBuffer, GetPacketStatus and message creation in a try/finally and call StartReceiving() from the finally block so RX is always restarted even on exceptions (keep existing early returns for IRQ timeout/CRC/absent RxDone before the try), referencing the HandleRxDone method and the StartReceiving, GetRxBufferStatus, ReadBuffer, and GetPacketStatus calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devices/LoRa/LoRa/ILoRaDevice.cs`:
- Around line 31-40: Update the ILoRaDevice.Send XML docs to include the payload
validation contract: state that implementations (e.g., Sx1262.SendCore) will
throw ArgumentNullException when payload is null, ArgumentException when
payload.Length == 0, and ArgumentOutOfRangeException when payload exceeds the
device's maximum allowed payload size (refer to the device-specific max payload
constant/property used by Sx1262.SendCore). Also mention that callers must
ensure payload is non-null, non-empty and within the device maximum before
calling Send, in addition to the existing timeout and thread-related exceptions.
In `@devices/LoRa/README.md`:
- Around line 1-13: The README is too terse—expand it to follow repo conventions
by adding a short description of the SX1262 chip and what Iot.Device.LoRa
provides, a wiring table or a link to the SX1262 datasheet, and a minimal usage
snippet that shows SPI initialization followed by Sx1262.Reset() →
Sx1262.Initialize() → StartPolling() / Send() (or ILoRaDevice usage), plus a
direct link to samples/Sx1262Sample and notes about the tests/ LoRaTests
hardware-free checks; ensure names like Iot.Device.LoRa, Sx1262, ILoRaDevice,
Reset(), Initialize(), StartPolling(), Send(), samples/Sx1262Sample, and tests/
are referenced so readers can find code and examples.
In `@devices/LoRa/samples/Sx1262Sample/Program.cs`:
- Around line 37-80: The Main setup is unprotected against exceptions from
_lora.Reset(), _lora.Initialize(), and _lora.StartPolling(); wrap the LoRa
initialization and start sequence in a try/catch that catches Exception, logs
the exception details via Debug.WriteLine (including ex.ToString()), and
performs graceful cleanup (dispose/stop polling/unsubscribe PacketReceived) or
implement a retry loop as appropriate; ensure the catch is around the block that
creates/configures Sx1262 (references: Main, SpiDevice.Create, new Sx1262,
_lora.Reset, _lora.Initialize, _lora.StartPolling, and _lora.PacketReceived) so
hardware failures produce a clear diagnostic message instead of an unhandled
crash.
- Around line 87-88: The code currently builds payload bytes then decodes them
immediately for logging; instead create a single message string (e.g., var
message = $"Hello from the .NET nanoFramework: {DateTime.UtcNow}") use
Debug.WriteLine(message) and only then call Encoding.UTF8.GetBytes(message) to
produce the byte[] payload; update references to payload and the log to use
these names and remove the redundant Encoding.UTF8.GetString call.
In `@devices/LoRa/tests/Properties/AssemblyInfo.cs`:
- Line 14: Stale copyright year in the AssemblyCopyright attribute — update the
string literal in the AssemblyCopyright attribute (assembly:
AssemblyCopyright("Copyright (c) 2021 nanoFramework contributors")) to the
current year or project convention (e.g., "Copyright (c) 2026 nanoFramework
contributors" or a range like "2021-2026") so the AssemblyInfo.cs reflects the
correct copyright period.
---
Duplicate comments:
In `@devices/LoRa/LoRa/Drivers/Sx1262.cs`:
- Around line 473-506: Access to _pollThread and transitions between
StartPolling/StopPolling/PollLoop are unsynchronized; add a private readonly
object (e.g., _pollLock) and use lock(_pollLock) around all reads/writes of
_pollThread and any start/stop transitions (StartPolling, StopPolling, PollLoop
and SendCore where _pollThread is checked) so thread A cannot observe stale or
racy state. Keep using Interlocked/Volatile for _stopPolling (or use
Volatile.Read/Write) but perform assignments and null-checks of _pollThread
inside the same lock and set _pollThread = null while holding the lock; apply
the same locking pattern to the other occurrence noted (the region around the
other 578-607 methods).
- Around line 422-451: The HandleRxDone method can throw from GetRxBufferStatus,
ReadBuffer, or GetPacketStatus and currently StartReceiving is skipped, leaving
the radio out of RX; wrap the block that calls GetRxBufferStatus, ReadBuffer,
GetPacketStatus and message creation in a try/finally and call StartReceiving()
from the finally block so RX is always restarted even on exceptions (keep
existing early returns for IRQ timeout/CRC/absent RxDone before the try),
referencing the HandleRxDone method and the StartReceiving, GetRxBufferStatus,
ReadBuffer, and GetPacketStatus calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b99f113e-8d6d-49f3-bec5-7e446e4353fb
📒 Files selected for processing (19)
devices/LoRa/LoRa.slndevices/LoRa/LoRa/Drivers/Sx1262.csdevices/LoRa/LoRa/ILoRaDevice.csdevices/LoRa/LoRa/LoRa.nfprojdevices/LoRa/README.mddevices/LoRa/Settings.StyleCopdevices/LoRa/samples/Sx1262Sample/Program.csdevices/LoRa/samples/Sx1262Sample/Properties/AssemblyInfo.csdevices/LoRa/samples/Sx1262Sample/Sx1262Sample.nfprojdevices/LoRa/samples/Sx1262Sample/packages.configdevices/LoRa/samples/Sx1262Sample/packages.lock.jsondevices/LoRa/tests/LoRaMessageTests.csdevices/LoRa/tests/LoRaTests.nfprojdevices/LoRa/tests/Properties/AssemblyInfo.csdevices/LoRa/tests/Sx1262ConstructorTests.csdevices/LoRa/tests/Sx1262DecodeChipModeTests.csdevices/LoRa/tests/nano.runsettingsdevices/LoRa/tests/packages.configdevices/LoRa/tests/packages.lock.json
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devices/LoRa/LoRa/Drivers/Sx1262.cs`:
- Around line 468-478: The PacketReceived event delegate is read twice which can
race to null between the check and invocation; fix it by copying PacketReceived
to a local variable (e.g., var handler = PacketReceived) then check handler !=
null and invoke handler(this, msg) inside the try/catch, leaving the
Debug.WriteLine in the catch; update the block around the current PacketReceived
invocation (the code that checks msg != null && PacketReceived != null and calls
PacketReceived(this, msg)) to use the local snapshot instead.
- Around line 150-166: The constructor in Sx1262.cs can leak GPIO resources if
GpioController.OpenPin throws; modify the Sx1262 constructor (the block that
assigns _gpio, _shouldDispose, _disposeSpi and calls _gpio.OpenPin for
_resetPin, _busyPin, _dio1Pin) to wrap the OpenPin calls in a try/catch/finally:
open pins sequentially inside try, and if any OpenPin throws, dispose any pins
already opened (_resetPin, _busyPin, _dio1Pin as applicable) and dispose the
owned GpioController when _shouldDispose is true, then rethrow the exception;
ensure fields are only left assigned to opened Pin objects after successful open
so Dispose() won't miss them.
- Around line 273-323: The poll-thread identity check that throws when Send is
called from the RX polling thread must be moved to execute before acquiring
_sendLock so a poll-thread handler won't deadlock waiting for that lock; update
Send (or the start of SendCore) to take _pollLock, check Thread.CurrentThread ==
_pollThread and throw immediately if true, capture wasPolling there, then
proceed to lock _sendLock and call SendCore (or continue) as before; keep using
_pollLock, _sendLock, _pollThread, StartPolling/StopPolling semantics unchanged
so StopPolling/worker.Join can complete without the deadlock.
In `@devices/LoRa/tests/packages.config`:
- Around line 1-8: Tests and sample projects are missing StyleCop: add the NuGet
package entry for StyleCop.MSBuild version 6.2.0 to LoRa/tests/packages.config
and LoRa/samples/Sx1262Sample/packages.config, then mirror the library's .nfproj
settings in LoRaTests.nfproj and Sx1262Sample.nfproj by adding the
StyleCopTreatErrorsAsWarnings property, the StyleCopOverrideSettingsFile
pointing to Settings.StyleCop, and an Import of the StyleCop.MSBuild targets so
that the build uses the same StyleCop.MSBuild configuration as the LoRa library
project.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f197004-685a-4603-bd04-03b5988cb9ee
📒 Files selected for processing (8)
devices/LoRa/LoRa/Drivers/Sx1262.csdevices/LoRa/LoRa/ILoRaDevice.csdevices/LoRa/README.mddevices/LoRa/samples/Sx1262Sample/Program.csdevices/LoRa/tests/LoRaTests.nfprojdevices/LoRa/tests/Properties/AssemblyInfo.csdevices/LoRa/tests/packages.configdevices/LoRa/tests/packages.lock.json
Added LoRa (Iot.Device.LoRa) Semtech SX1262 transceiver to the wireless communication modules section in the README, including a NuGet badge and relevant documentation links.
Author
|
@Ellerbach Hi Laurent, I have the PR in a better state now. |
Ellerbach
reviewed
Apr 28, 2026
Member
Ellerbach
left a comment
Ellerbach
left a comment
There was a problem hiding this comment.
Thanks, really cool! Couple of comments and suggestions to improve few things.
| busyPinObj = null; | ||
| dio1PinObj = null; | ||
| } | ||
| catch |
Member
There was a problem hiding this comment.
why not just testing the validity of the pins?
Author
There was a problem hiding this comment.
@Ellerbach I may be overcomplicating this, so I’d appreciate your take. The intent of the try/catch is: if OpenPin fails partway through, we dispose any pins we already opened so the GPIO controller doesn’t keep them reserved—making a retry in the same session (or tests) safer. I’m not assuming the app always recovers without a reboot on real hardware.
Member
There was a problem hiding this comment.
ok but you should not set them to null in the try part!
and rather than checking if the pins are null, then call Dispose() (you may have to adjust it a bit for the stoppolling thread)
| _resetPin = resetPinObj; | ||
| _busyPin = busyPinObj; | ||
| _dio1Pin = dio1PinObj; | ||
| resetPinObj = null; |
Member
There was a problem hiding this comment.
why do you set them to null in here? You just tried to set them up before?
|
|
||
| * [](https://www.nuget.org/packages/nanoFramework.Iot.Device.AtModem/) [Generic AT Modem SIM800 and SIM7070, SIM7080, SIM7090 - Dual Mode Wireless Module CatM, LTE modems](AtModem) | ||
| * [](https://www.nuget.org/packages/nanoFramework.Iot.Device.Nrf24l01/) [nRF24L01 - Single Chip 2.4 GHz Transceiver](Nrf24l01) | ||
| * [](https://www.nuget.org/packages/nanoFramework.Iot.Device.LoRa/) [LoRa - Semtech SX1262 transceiver (Iot.Device.LoRa)](LoRa) |
Member
There was a problem hiding this comment.
that's not needed :-) it's added automatically at merge :-)
Member
There was a problem hiding this comment.
But you are missing the category.txt file. Se https://github.com/nanoframework/nanoFramework.IoT.Device/blob/main/devices/A4988/category.txt
And for the list, pick existing ones: https://github.com/nanoframework/nanoFramework.IoT.Device/blob/main/src/device-listing/categories.json
If you really want a new one, add it to the list.
Author
There was a problem hiding this comment.
@Ellerbach i have the README and category file in the root of the project, is that perhaps the wrong place to have the files? Where should they be?
Member
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
|
Please remove the pdf, we are not checking in datasheet. Most are proprietary of their owner. But you can point out where it is located on the device producer. |
Ellerbach
reviewed
May 8, 2026
Member
Ellerbach
left a comment
Ellerbach
left a comment
There was a problem hiding this comment.
Thanks, just a change needed in the constructor to avoid the pins open to get disposed by the garbage collector in the try, removing the pdf file and we'll be good to go!
| busyPinObj = null; | ||
| dio1PinObj = null; | ||
| } | ||
| catch |
Member
There was a problem hiding this comment.
ok but you should not set them to null in the try part!
and rather than checking if the pins are null, then call Dispose() (you may have to adjust it a bit for the stoppolling thread)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Iot.Device.LoRalibrary for LoRa radio support.ILoRaDevicebase interface to provide a common abstraction for LoRa radio implementations.Sx1262driver implementation for SPI-based SX1262 LoRa transceivers.Sx1262SamplenanoFramework sample application showing initialization, transmit, and receive polling.ReadBufferSPI read handling so the driver skips the protocol bytes correctly and returns only the received payload.Motivation and Context
devicesfolder and provides a sample to validate and demonstrate usage.How Has This Been Tested?
Sx1262Sampleapplication.ReadBufferfix by confirming that received payloads were returned correctly in the sample output.01/01/1970timestamps in the debug output are expected for this test setup because the device clock was not set.Screenshots
Sx1262Sampleshowing successful startup, initialization, RX polling, TX messages, and received packets with RSSI/SNR values.Types of changes
Checklist: