Esp32 RMT changes for latest IDF api - #128
Merged
Merged
Conversation
nfbot
added
Type: enhancement
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: nanoframework/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates the ESP32 RMT managed API to V3. It adds symbols, encoders, decoding, LED transmission, synchronized transmission, resolution-based settings, continuous reception, expanded tests, and V3 documentation. ChangesRMT V3 API
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant TransmitterChannel
participant TransmitterEncodedChannel
participant NativeRmt
Application->>TransmitterChannel: Send(RmtSymbols)
TransmitterChannel->>RmtSymbols: Serialize()
TransmitterChannel->>TransmitterEncodedChannel: SendData(serialized symbols)
TransmitterEncodedChannel->>NativeRmt: Start transmission
NativeRmt-->>TransmitterEncodedChannel: Return channel state
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nanoFramework.Hardware.Esp32.Rmt/TransmitChannelSettings.cs (1)
161-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe copy constructor drops
CarrierAlwaysOn.The copy constructor copies every other field but omits
_carrierAlwaysOn, so the copy silently reverts tofalse. This constructor snapshots the settings for the channel, so a caller that setsCarrierAlwaysOn = trueloses the setting and the carrier stops being emitted while idle.
ReceiverChannelSettingscopies its newly added_bufferSizefield at line 205. Apply the same treatment here.🐛 Proposed fix
_enableCarrierWave = other.EnableCarrierWave; _carrierLevel = other.CarrierLevel; _carrierWaveFrequency = other.CarrierWaveFrequency; _carrierWaveDutyPercentage = other.CarrierWaveDutyPercentage; + _carrierAlwaysOn = other.CarrierAlwaysOn; _enableLooping = other.EnableLooping;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/TransmitChannelSettings.cs` around lines 161 - 177, Update the TransmitChannelSettings copy constructor to copy the CarrierAlwaysOn value into _carrierAlwaysOn, matching the handling of the other carrier-related fields and the _bufferSize copy in ReceiverChannelSettings.nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs (1)
117-136: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe duty-cycle guard blocks the fractional values that
floatenables, and contradicts its own documentation.The type changed from
bytetofloat, but the guard still rejects every value below 1. A duty cycle of 0.5 percent throws, so the type change gives no new range. Line 120 documents the limit as "cannot be less that 0", which does not match the guard.
TransmitChannelSettings.CarrierWaveDutyPercentageusesvalue <= 0for the same concept. Align both classes.ReceiverChannelSettings_ThrowsOnInvalidValuesatRmtObjectTests.csline 160 asserts that 0 throws, andvalue <= 0keeps that assertion true.🐛 Proposed fix
- /// <exception cref="ArgumentOutOfRangeException">Value cannot be less that 0 or greater than 100.</exception> + /// <exception cref="ArgumentOutOfRangeException">Value cannot be 0 or less, or greater than 100.</exception> /// <remarks> /// This configuration is not available on the base ESP32 target and will be ignored. Please refer to the ESP32 IDF docs for more information on feature availability for the various ESP32 targets. /// </remarks> public float CarrierWaveDutyPercentage { get => _carrierWaveDutyPercentage; set { - if (value < 1 || value > 100) + if (value <= 0 || value > 100) { throw new ArgumentOutOfRangeException(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs` around lines 117 - 136, Update the CarrierWaveDutyPercentage setter in ReceiverChannelSettings to reject values less than or equal to zero, while still rejecting values above 100, matching TransmitChannelSettings and preserving the existing zero-value test. Keep fractional positive float values such as 0.5 valid and correct the exception documentation wording to reflect the enforced range.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nanoFramework.Hardware.Esp32.Rmt.Benchmarks/SerializeCommandsBenchmark.cs`:
- Around line 60-64: Update SerializeCommands_Current to invalidate the cached
serialized result before each call by reassigning an element through
RmtSymbolsArrayList’s public indexer, whose setter clears the cache, then invoke
Serialize so every benchmark iteration measures regeneration rather than a cache
hit.
In `@nanoFramework.Hardware.Esp32.Rmt.UnitTests/RmtObjectTests.cs`:
- Around line 31-52: Add a unit test alongside RmtSymbols_AddClearIndexing_Works
that serializes after adding an item, verifies the cached output expands after
another Add, then replaces an indexed symbol and verifies Serialize reflects the
replacement. Use RmtSymbols.Serialize and the existing Add/indexer APIs to cover
cache invalidation through both mutation paths.
In `@nanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.cs`:
- Around line 41-47: Rename the ByteEncoderSettings constructor parameters Bit0,
Bit1, and MsbFirst to camelCase names consistent with byteLength, and update
their assignments and XML documentation references while preserving the existing
public constructor behavior.
In `@nanoFramework.Hardware.Esp32.Rmt/CopyEncoderSettings.cs`:
- Around line 12-15: Update the XML summary above CopyEncoderSettings to
describe settings for creating a copy encoder instead of a byte encoder; leave
the class declaration and other documentation unchanged.
- Around line 19-40: Update CopyEncoderSettings by adding an explicit
parameterless constructor, removing the default value from the RmtSymbols
constructor parameter, and retaining the RmtSymbol[] overload without ambiguity
for null arguments. Fill in meaningful descriptions for the symbols parameters
in both constructor XML documentation blocks, while preserving existing
initialization and serialization behavior.
In `@nanoFramework.Hardware.Esp32.Rmt/EncoderData.cs`:
- Around line 20-29: Correct the EncoderData XML documentation typo from “ti” to
“to”, and update the EncoderData constructor’s loop validation so values below 1
are rejected before assigning Loop, matching the range-guard behavior of
TransmitChannelSettings.LoopCount and ReceiverChannelSettings.IdleThreshold.
In `@nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs`:
- Around line 159-170: The SendLedData method passes only one EncoderData entry
despite _transmitChannel being configured with two encoders. Update its
SendWithEncoders call to provide a second EncoderData entry for the reset
CopyEncoder, such as an empty/null-data entry, while preserving the existing LED
data and repeat-count entry.
In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannel.cs`:
- Around line 103-113: Update Receive() to mirror the null/empty handling in
TryGetReceivedSymbols(): inspect NativeReceive()’s result and return null when
it is null or empty; otherwise wrap the received symbols in RmtSymbols. Preserve
the documented blocking behavior and non-empty result handling.
In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs`:
- Around line 59-74: Update ReceiverChannelSettings.FilterThreshold to validate
values in its setter and throw ArgumentOutOfRangeException for values rejected
by the native layer, preserving the documented exception contract; in
nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs lines 59-74, make
this change at the FilterThreshold property. In the same file lines 151-162,
update BufferSize to reject values below 1, and add its default value of 100 to
the constructor documentation list at lines 168-177.
In `@nanoFramework.Hardware.Esp32.Rmt/RmtSymbol.cs`:
- Around line 108-109: Update the XML summary for MaxDuration/CheckDuration in
RmtSymbol to document the maximum supported duration as 32767 instead of 32768,
matching the validation boundary.
- Around line 43-50: Fill the XML documentation for the public RmtSymbol
constructor parameters: describe duration0 and duration1 as channel-tick
durations whose time depends on ResolutionHz, and describe level0 and level1 as
the corresponding signal levels. Keep the constructor signature and behavior
unchanged.
In `@nanoFramework.Hardware.Esp32.Rmt/RmtSymbols.cs`:
- Around line 52-55: Restrict or remove the public Symbols property in
RmtSymbols so callers cannot mutate the live _symbols list without invalidating
_nativeSymbolsBuffer; retain foreach support through GetEnumerator. If Symbols
is made internal, update SerializeCommandsBenchmark to enumerate
RmtSymbolsArrayList directly instead of accessing Symbols.
- Around line 63-88: Update both accessors of the RmtSymbols indexer to
explicitly reject negative index values as well as indices beyond the collection
count, throwing IndexOutOfRangeException before accessing _symbols or modifying
_nativeSymbolsBuffer. Preserve the existing valid-index behavior.
In `@nanoFramework.Hardware.Esp32.Rmt/RmtUtils.cs`:
- Around line 52-55: Update the return documentation for the decoding method in
RmtUtils so the byte-array length uses the effective symbol count after
resolving the -1 “decode all remaining symbols” sentinel, rather than directly
using symbolLength. Also correct the nearby RmtSymbol description to identify it
as a class instead of a structure.
- Around line 13-17: Rename the public static class Utils to RmtUtils so the
type matches the RmtUtils file and namespace context. Update all references to
Utils throughout the project and preserve the existing utility members and
behavior.
In `@nanoFramework.Hardware.Esp32.Rmt/TransmiterEncodedChannel.cs`:
- Line 1: Rename TransmiterEncodedChannel.cs to TransmitterEncodedChannel.cs and
update the corresponding nfproj file reference, keeping the existing
TransmitterEncodedChannel class unchanged.
In `@nanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.cs`:
- Around line 23-36: Add null validation to the TransmitSyncManager constructor
for both TxChannelArray and each TransmitterChannel element before accessing
Length or Handle, throwing ArgumentNullException with the relevant parameter
name for either null case; preserve the existing handle collection and
NativeCreateSyncManager flow for valid input.
In `@nanoFramework.Hardware.Esp32.Rmt/TransmitterChannel.cs`:
- Around line 22-88: Remove the entire commented-out property block from
TransmitterChannel.cs, including IsChannelIdle, Handle, EnableLooping,
LoopCount, CarrierLevel, and IdleLevel, since these members are inherited from
TransmitterEncodedChannel.
- Around line 106-124: Add null validation to TransmitterChannel.Send for
symbols and SendData for data, throwing ArgumentNullException before calling
Serialize or NativeTxWriteSymbolItems; preserve the existing waitTxDone behavior
for valid inputs.
In `@README.md`:
- Around line 93-95: In the README text describing memory block sizes, update
the phrase “more then one” to “more than one” while leaving the surrounding
explanation unchanged.
- Around line 61-67: Update the README RmtSymbol sample to use the constructor’s
actual named parameters: duration0 and level0 for the first pair, followed by
duration1 and level1 for the second pair; remove the nonexistent duration2 and
level2 names while preserving the example values and intent.
---
Outside diff comments:
In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs`:
- Around line 117-136: Update the CarrierWaveDutyPercentage setter in
ReceiverChannelSettings to reject values less than or equal to zero, while still
rejecting values above 100, matching TransmitChannelSettings and preserving the
existing zero-value test. Keep fractional positive float values such as 0.5
valid and correct the exception documentation wording to reflect the enforced
range.
In `@nanoFramework.Hardware.Esp32.Rmt/TransmitChannelSettings.cs`:
- Around line 161-177: Update the TransmitChannelSettings copy constructor to
copy the CarrierAlwaysOn value into _carrierAlwaysOn, matching the handling of
the other carrier-related fields and the _bufferSize copy in
ReceiverChannelSettings.
🪄 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 Plus
Run ID: ebb9bf8a-7429-4073-b9bb-aacb7b9f87a5
📒 Files selected for processing (29)
README.mdnanoFramework.Hardware.Esp32.Rmt.Benchmarks/Program.csnanoFramework.Hardware.Esp32.Rmt.Benchmarks/SerializeCommandsBenchmark.csnanoFramework.Hardware.Esp32.Rmt.UnitTests/RmtCommandSerializerTests.csnanoFramework.Hardware.Esp32.Rmt.UnitTests/RmtObjectTests.csnanoFramework.Hardware.Esp32.Rmt.UnitTests/nanoFramework.Hardware.Esp32.Rmt.UnitTests.nfprojnanoFramework.Hardware.Esp32.Rmt.slnnanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.csnanoFramework.Hardware.Esp32.Rmt/ChannelMode.csnanoFramework.Hardware.Esp32.Rmt/CopyEncoderSettings.csnanoFramework.Hardware.Esp32.Rmt/EncoderData.csnanoFramework.Hardware.Esp32.Rmt/EncoderSettings.csnanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.csnanoFramework.Hardware.Esp32.Rmt/Properties/AssemblyInfo.csnanoFramework.Hardware.Esp32.Rmt/ReceiverChannel.csnanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.csnanoFramework.Hardware.Esp32.Rmt/RmtChannel.csnanoFramework.Hardware.Esp32.Rmt/RmtChannelSettings.csnanoFramework.Hardware.Esp32.Rmt/RmtCommandSerializer.csnanoFramework.Hardware.Esp32.Rmt/RmtSymbol.csnanoFramework.Hardware.Esp32.Rmt/RmtSymbols.csnanoFramework.Hardware.Esp32.Rmt/RmtUtils.csnanoFramework.Hardware.Esp32.Rmt/SourceClock.csnanoFramework.Hardware.Esp32.Rmt/TransmitChannelSettings.csnanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.csnanoFramework.Hardware.Esp32.Rmt/TransmiterEncodedChannel.csnanoFramework.Hardware.Esp32.Rmt/TransmitterChannel.csnanoFramework.Hardware.Esp32.Rmt/nanoFramework.Hardware.Esp32.Rmt.nfprojversion.json
💤 Files with no reviewable changes (4)
- nanoFramework.Hardware.Esp32.Rmt/SourceClock.cs
- nanoFramework.Hardware.Esp32.Rmt/ChannelMode.cs
- nanoFramework.Hardware.Esp32.Rmt.UnitTests/RmtCommandSerializerTests.cs
- nanoFramework.Hardware.Esp32.Rmt/RmtCommandSerializer.cs
Member
|
| Previously published | This build | |
|---|---|---|
| Native version | 100.0.5.1 |
100.0.6.0 |
| Checksum | 0x8ADAC728 |
0x84E4A55C |
👉 Tracking issue: nanoframework/Home#1825
The issue contains a link to the stubs artifact from this build and instructions for GitHub Copilot to perform the update.
13 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
README.md (7)
130-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the non-blocking receive example so it compiles and exits.
Line 132 is missing a semicolon. Line 137 contains an extra
). The unconditionalwhile (true)also makesrx.Stop()unreachable. Add an application-specific exit condition or cancellation path.Proposed syntax fix
- RmtSymbols received = rx.TryGetReceivedSymbols() + RmtSymbols received = rx.TryGetReceivedSymbols(); ... -}) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 130 - 139, Fix the non-blocking receive example by adding the missing semicolon after TryGetReceivedSymbols(), removing the extra closing parenthesis after the loop, and replacing the unconditional while(true) with an application-specific exit condition or cancellation path so rx.Stop() is reachable.
192-202: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the declared
CopyEncoderSettingsvariable.Line 194 references
copyEnc, but the Copy Encoder example declarescopyon Line 167. The encoder pipeline does not compile when these snippets are combined. Use one variable name consistently.Proposed fix
var tx = new TransmitterEncodedChannel( settings, - new EncoderSettings[] { byteEnc, copyEnc }); + new EncoderSettings[] { byteEnc, copy });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 192 - 202, Update the TransmitterEncodedChannel construction to use the already-declared CopyEncoderSettings variable name consistently, replacing the undefined copyEnc reference with copy while leaving the rest of the example unchanged.
317-318: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the units for custom LED timings.
The custom
LedTransmitChannelconstructor expectsT0L,T0H,T1L,T1H, andResetTimein 100-nanosecond ticks. Lines 317-318 only mention that custom timings can be passed. Add the units and parameter order to prevent incorrect LED waveforms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 317 - 318, Update the README guidance for the custom LedTransmitChannel constructor to state that timing arguments are specified in 100-nanosecond ticks and list the parameter order as T0L, T0H, T1L, T1H, ResetTime.
205-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the encoder API name and null-data description.
Line 206 uses
SendWithEncoder, but the API call on Line 202 isSendWithEncoders. Line 209 describes a nullEncoderDataobject, while Line 199 creates anEncoderDataobject whoseDataproperty is null. Document theEncoderData.Data == nullbehavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 205 - 210, Update the README text around EncoderSettings and SendWithEncoders to use the plural API name consistently, and describe the null-data case as EncoderData.Data being null rather than EncoderData itself. Preserve the explanation that no data is supplied when the encoder already contains embedded data.
252-259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefine the synchronized payloads before sending.
The sample uses
symbols1,symbols2, andsymbols3on Lines 256-258 without declaring them. Add theRmtSymbolsvalues or state that they are prepared before this snippet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 252 - 259, Update the “Starting a Synchronized Transmission” example to define or initialize the RmtSymbols values symbols1, symbols2, and symbols3 before the Send calls, or explicitly state that these payloads must be prepared beforehand.
359-361: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the documented utility class name.
DecodeSymbolArrayToBytes()is declared on the publicRmtUtilsclass, notUtils. UpdateREADME.md:359toRmtUtils.DecodeSymbolArrayToBytes()so the example compiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 359 - 361, Update the README utility reference from Utils.DecodeSymbolArrayToBytes() to RmtUtils.DecodeSymbolArrayToBytes(), matching the public RmtUtils class declaration and keeping the example compilable.
277-286: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse encoder-backed channels for synchronized encoded LED output.
TransmitSyncManageronly linksTransmitterChannelinstances, and the sample passes byte arrays withSendWithEncodersbut does not configureByteEncoderSettings. Construct WS2812/SK6812 data asRmtSymbols, then call synchronizedSend(...), or extend synchronization to support configuredTransmitterEncodedChannelinstances.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 277 - 286, Update the README synchronization example around TransmitSyncManager to use supported encoded-channel synchronization: either construct WS2812/SK6812 output as RmtSymbols and call synchronized Send(...), or configure and synchronize TransmitterEncodedChannel instances. Do not pass raw byte arrays through SendWithEncoders without ByteEncoderSettings.nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs (1)
123-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
float.NaNforCarrierWaveDutyPercentagein both settings classes.Ordered comparisons accept
float.NaN, so this setter stores a value outside the documented(0, 100]range. Use an inclusive valid-range check in bothnanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.csandnanoFramework.Hardware.Esp32.Rmt/TransmitChannelSettings.cs.Proposed fix
- if (value <= 0 || value > 100) + if (!(value > 0 && value <= 100))Add coverage for non-finite input values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs` around lines 123 - 135, Update the CarrierWaveDutyPercentage setters in ReceiverChannelSettings.cs (lines 123-135) and TransmitChannelSettings.cs (lines 69-81) to reject NaN by using an inclusive valid-range check for (0, 100]. Add coverage for NaN and other non-finite inputs in the relevant tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.cs`:
- Around line 44-52: Validate byteLength in the ByteEncoderSettings constructor
before assigning _byteLen, rejecting values below zero while preserving 0 as the
“all remaining data” sentinel. Add a test covering construction with -1 and
verify it fails with the expected argument-validation behavior.
In `@nanoFramework.Hardware.Esp32.Rmt/RmtSymbols.cs`:
- Around line 53-55: Update the RmtSymbols serialization flow so mutations to
any contained RmtSymbol invalidate or bypass the cached serialized bytes,
including symbols accessed through Symbols or retained after Add(). Remove the
cache if necessary, or connect RmtSymbol property changes to the owning
collection, and add a test that changes a symbol after the first Serialize()
call and verifies the new waveform is returned.
---
Outside diff comments:
In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs`:
- Around line 123-135: Update the CarrierWaveDutyPercentage setters in
ReceiverChannelSettings.cs (lines 123-135) and TransmitChannelSettings.cs (lines
69-81) to reject NaN by using an inclusive valid-range check for (0, 100]. Add
coverage for NaN and other non-finite inputs in the relevant tests.
In `@README.md`:
- Around line 130-139: Fix the non-blocking receive example by adding the
missing semicolon after TryGetReceivedSymbols(), removing the extra closing
parenthesis after the loop, and replacing the unconditional while(true) with an
application-specific exit condition or cancellation path so rx.Stop() is
reachable.
- Around line 192-202: Update the TransmitterEncodedChannel construction to use
the already-declared CopyEncoderSettings variable name consistently, replacing
the undefined copyEnc reference with copy while leaving the rest of the example
unchanged.
- Around line 317-318: Update the README guidance for the custom
LedTransmitChannel constructor to state that timing arguments are specified in
100-nanosecond ticks and list the parameter order as T0L, T0H, T1L, T1H,
ResetTime.
- Around line 205-210: Update the README text around EncoderSettings and
SendWithEncoders to use the plural API name consistently, and describe the
null-data case as EncoderData.Data being null rather than EncoderData itself.
Preserve the explanation that no data is supplied when the encoder already
contains embedded data.
- Around line 252-259: Update the “Starting a Synchronized Transmission” example
to define or initialize the RmtSymbols values symbols1, symbols2, and symbols3
before the Send calls, or explicitly state that these payloads must be prepared
beforehand.
- Around line 359-361: Update the README utility reference from
Utils.DecodeSymbolArrayToBytes() to RmtUtils.DecodeSymbolArrayToBytes(),
matching the public RmtUtils class declaration and keeping the example
compilable.
- Around line 277-286: Update the README synchronization example around
TransmitSyncManager to use supported encoded-channel synchronization: either
construct WS2812/SK6812 output as RmtSymbols and call synchronized Send(...), or
configure and synchronize TransmitterEncodedChannel instances. Do not pass raw
byte arrays through SendWithEncoders without ByteEncoderSettings.
🪄 Autofix
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 Plus
Run ID: 28c4508d-3f3a-4a6c-9496-130338e82de6
📒 Files selected for processing (17)
README.mdnanoFramework.Hardware.Esp32.Rmt.Benchmarks/SerializeCommandsBenchmark.csnanoFramework.Hardware.Esp32.Rmt.UnitTests/RmtObjectTests.csnanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.csnanoFramework.Hardware.Esp32.Rmt/CopyEncoderSettings.csnanoFramework.Hardware.Esp32.Rmt/EncoderData.csnanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.csnanoFramework.Hardware.Esp32.Rmt/ReceiverChannel.csnanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.csnanoFramework.Hardware.Esp32.Rmt/RmtSymbol.csnanoFramework.Hardware.Esp32.Rmt/RmtSymbols.csnanoFramework.Hardware.Esp32.Rmt/RmtUtils.csnanoFramework.Hardware.Esp32.Rmt/TransmitChannelSettings.csnanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.csnanoFramework.Hardware.Esp32.Rmt/TransmitterChannel.csnanoFramework.Hardware.Esp32.Rmt/TransmitterEncodedChannel.csnanoFramework.Hardware.Esp32.Rmt/nanoFramework.Hardware.Esp32.Rmt.nfproj
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs (2)
148-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve odd
ResetTimevalues.
ResetTime / 2truncates. ForResetTime = 1, both symbol durations become zero. Every odd value produces a reset pulse shorter than requested. Split the duration into two parts whose sum equalsResetTime, or reject odd values.Proposed fix
- ushort resetHalf = (ushort)(ResetTime / 2); - RmtSymbol[] resetSymbols = new[] { new RmtSymbol(resetHalf, false, resetHalf, false) }; + ushort resetFirst = (ushort)(ResetTime / 2); + ushort resetSecond = (ushort)(ResetTime - resetFirst); + RmtSymbol[] resetSymbols = new[] { new RmtSymbol(resetFirst, false, resetSecond, false) };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs` around lines 148 - 149, Update the reset-symbol construction near resetHalf in LedTransmitChannel so odd ResetTime values are preserved: calculate two duration parts whose sum exactly equals ResetTime and use them for the two RmtSymbol durations, while retaining the existing reset polarity behavior.
172-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the
waitToCompleteXML documentation.The parameter text is incomplete. State that
truewaits for transmission completion andfalsereturns before completion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs` at line 172, Complete the XML documentation for the waitToComplete parameter in LedTransmitChannel by stating that true waits for transmission completion, while false returns before transmission completes.nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs (1)
60-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQualify the
FilterThresholdexample by target and resolution.The documentation recommends
100000ns, then states that an 80 MHz clock supports at most3199ns. Without a target or resolution qualifier, users can copy an invalid value and receive the documented start-time failure. Use an example within the 80 MHz limit or state which hardware and resolution support100000ns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs` around lines 60 - 76, Update the FilterThreshold documentation to qualify the 100000 ns example with the target and clock resolution that support it, or replace it with an example valid for the documented 80 MHz limit. Ensure the example does not contradict the maximum-value guidance or imply that 100000 ns is universally accepted.nanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.cs (2)
36-38: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIterate over
TransmitterEncodedChannel.
TxChannelArrayis declared asTransmitterEncodedChannel[], but the loop casts each element toTransmitterChannel. SinceTransmitterEncodedChannelis not a subclass ofTransmissionChannel, creating a sync manager for LED channels throwsInvalidCastExceptionbeforeNativeCreateSyncManager. ReadHandlethroughTransmitterEncodedChannel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.cs` around lines 36 - 38, The loop initializing channelHandles should iterate over TxChannelArray elements as TransmitterEncodedChannel rather than TransmitterChannel. Update the foreach variable type in the sync-manager initialization so it reads each encoded channel’s Handle without casting, allowing NativeCreateSyncManager to be reached.
48-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject
Reset()after disposal.
Dispose()callsNativeDisposeSyncManager()before settingdisposedValue, so a laterReset()can still callNativeResetSyncManager(). Use the sync-manager handle after native disposal, which is invalid after the underlying handle has been freed. ThrowObjectDisposedExceptionbefore the native reset call.Proposed fix
public void Reset() { + if (disposedValue) + { + throw new ObjectDisposedException(nameof(TransmitSyncManager)); + } + NativeResetSyncManager(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.cs` around lines 48 - 50, Update TransmitSyncManager.Reset to check the disposed state before calling NativeResetSyncManager, and throw ObjectDisposedException when the manager has been disposed. Preserve the existing native reset behavior for active instances.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.cs`:
- Around line 37-40: Update the XML documentation for
ByteEncoderSettings.ByteLength to describe it as the configured number of bytes
handled by the encoder, noting that a value of 0 means processing all remaining
data; do not describe it as a byte-array length.
In `@README.md`:
- Line 132: Update the receive polling loop around TryGetReceivedSymbols to use
a cancellation or termination condition instead of while(true), add a yield or
delay between polls, and ensure the loop can exit so the subsequent rx.Stop()
call is reachable.
- Around line 206-207: Update the SendWithEncoders documentation to state that
it accepts a single EncoderData[] containing one EncoderData entry per encoder,
with each entry supplying data to the corresponding encoder; remove wording that
describes it as an array of EncoderData[] values.
---
Outside diff comments:
In `@nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs`:
- Around line 148-149: Update the reset-symbol construction near resetHalf in
LedTransmitChannel so odd ResetTime values are preserved: calculate two duration
parts whose sum exactly equals ResetTime and use them for the two RmtSymbol
durations, while retaining the existing reset polarity behavior.
- Line 172: Complete the XML documentation for the waitToComplete parameter in
LedTransmitChannel by stating that true waits for transmission completion, while
false returns before transmission completes.
In `@nanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.cs`:
- Around line 60-76: Update the FilterThreshold documentation to qualify the
100000 ns example with the target and clock resolution that support it, or
replace it with an example valid for the documented 80 MHz limit. Ensure the
example does not contradict the maximum-value guidance or imply that 100000 ns
is universally accepted.
In `@nanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.cs`:
- Around line 36-38: The loop initializing channelHandles should iterate over
TxChannelArray elements as TransmitterEncodedChannel rather than
TransmitterChannel. Update the foreach variable type in the sync-manager
initialization so it reads each encoded channel’s Handle without casting,
allowing NativeCreateSyncManager to be reached.
- Around line 48-50: Update TransmitSyncManager.Reset to check the disposed
state before calling NativeResetSyncManager, and throw ObjectDisposedException
when the manager has been disposed. Preserve the existing native reset behavior
for active instances.
🪄 Autofix
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 Plus
Run ID: fd4b8d79-db0b-4f92-9b06-9168c50bbf17
📒 Files selected for processing (5)
README.mdnanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.csnanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.csnanoFramework.Hardware.Esp32.Rmt/ReceiverChannelSettings.csnanoFramework.Hardware.Esp32.Rmt/TransmitSyncManager.cs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.cs (1)
58-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the invalid parameter name in the exception.
new ArgumentOutOfRangeException()leavesParamNameunset. Usenameof(byteLength)so callers and tests can identify the invalid argument.Proposed fix
- throw new ArgumentOutOfRangeException(); + throw new ArgumentOutOfRangeException(nameof(byteLength));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.cs` around lines 58 - 60, Update the byteLength validation in ByteEncoderSettings to construct ArgumentOutOfRangeException with nameof(byteLength), ensuring the exception identifies the invalid parameter while preserving the existing validation behavior.nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs (1)
80-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject undefined
LedTypevalues before falling back to WS2812 timings.The current switch falls through to WS2812 timing for any unsupported
LedType, so cast values such as(LedType)999initialize with the wrong profile. Keepcase LedType.WS2812explicit and throwArgumentOutOfRangeExceptionfromdefault.Suggested fix
- default: case LedType.WS2812: T0L = 9; T0H = 3; T1L = 3; T1H = 9; ResetTime = 500; break; + default: + throw new ArgumentOutOfRangeException(nameof(wsType), wsType, "Unsupported LED type.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs` around lines 80 - 93, Update the switch on wsType in the LED timing initialization to keep LedType.WS2812 as an explicit case, and make the default branch throw ArgumentOutOfRangeException for unsupported values instead of applying WS2812 timings. Preserve the existing timing profiles for WS2811 and WS2812.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs`:
- Around line 148-150: Update the reset-symbol construction in the transmit flow
around ResetTime, resetFirst, and resetSecond so custom reset durations above
65534 are rejected before creating RmtSymbol. Preserve the existing split
calculation for valid values and ensure neither symbol duration exceeds
RmtSymbol’s 15-bit maximum.
In `@README.md`:
- Around line 130-132: Update the receive loop around RmtSymbols to declare and
update exit before it is used, connecting it to the application stop condition
so rx.Stop() is reachable; alternatively, replace it with a concrete
cancellation or timeout condition. Ensure the README sample compiles and cannot
rely on an undefined variable.
---
Outside diff comments:
In `@nanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.cs`:
- Around line 58-60: Update the byteLength validation in ByteEncoderSettings to
construct ArgumentOutOfRangeException with nameof(byteLength), ensuring the
exception identifies the invalid parameter while preserving the existing
validation behavior.
In `@nanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs`:
- Around line 80-93: Update the switch on wsType in the LED timing
initialization to keep LedType.WS2812 as an explicit case, and make the default
branch throw ArgumentOutOfRangeException for unsupported values instead of
applying WS2812 timings. Preserve the existing timing profiles for WS2811 and
WS2812.
🪄 Autofix
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 Plus
Run ID: 73752977-5b8c-40fa-97e8-fa048c538189
📒 Files selected for processing (3)
README.mdnanoFramework.Hardware.Esp32.Rmt/ByteEncoderSettings.csnanoFramework.Hardware.Esp32.Rmt/LedTransmitChannel.cs
josesimoes
approved these changes
Aug 19, 2026
josesimoes
left a comment
josesimoes
left a comment
Member
There was a problem hiding this comment.
Nice refactoring and rework! Only one comment.
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
14 tasks
|
14 tasks
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
3 participants
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
This is a rework of the RMT native driver for ESP32 to update, improve performance, and use the latest RMT API which is very different from original.
This PR is to be merged at same time as matching managed code library nanoFramework Hardware.Esp32.Rmt
The updated samples will be updated once NUGET & firmware is available.
See nanoframework/nf-interpreter#3510 for more detail
Motivation and Context
All peripheral API need to be updated to latest before we can move to IDF version 6.X as the old ones have been deprecated.
How Has This Been Tested?
Tested locally with various targets using the updated version of samples.
Types of changes
Checklist: