Add NeoPixel #44
Add NeoPixel #44
Conversation
📝 WalkthroughWalkthroughMultiple projects have been updated to integrate new button-driven functionality and visual demonstrations. In the RipTide.Nfirmware client, the Main method now detects a button press using a GpioButton and, when pressed, logs a message and runs a NeoPixelGauge demo. The associated project files, README, and packages have been updated with additional nanoFramework dependencies. In the Peripherals section, the Bluetooth sensor now uses ticks for timestamp handling, new IO.Peripherals references have been added, and a brand-new NeoPixel.Peripheral project (with its class, configuration, and assembly information) has been introduced. The solution file is updated to include this new project. Changes
Sequence Diagram(s)sequenceDiagram
participant P as Program.Main
participant B as GpioButton
participant D as Debug Logger
participant N as NeoPixelGauge
P->>B: Initialize Button (Pin 8, 50ms debounce)
B-->>P: Trigger ButtonDown event (set flag)
P->>P: Delay for 1 second
alt Button Pressed
P->>D: Log "Button pressed" message
P->>P: Sleep for 1 minute
end
P->>N: Instantiate NeoPixelGauge (45 pixels, specified colors)
N->>N: Execute DemoRun (cycle LED colors continuously)
P->>P: Enter infinite sleep state
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (5)
src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs (2)
77-81: Consider consistent timestamp handling.While using Ticks improves performance by avoiding DateTime allocations, the code mixes DateTime and Ticks usage. For instance,
args.Timestampis still a DateTime but is converted to Ticks for storage.Consider this refactor for consistency:
- var utcNow = DateTime.UtcNow.Ticks; - var expiry = _eventHistory[args.BluetoothAddress]; - if (expiry == null || (long)expiry < utcNow) - { - _eventHistory[args.BluetoothAddress] = args.Timestamp.Add(_retentionInterval).Ticks; - } + var utcNowTicks = DateTime.UtcNow.Ticks; + var expiryTicks = _eventHistory[args.BluetoothAddress]; + if (expiryTicks == null || (long)expiryTicks < utcNowTicks) + { + _eventHistory[args.BluetoothAddress] = args.Timestamp.Ticks + _retentionInterval.Ticks; + }
136-140: Consider a more descriptive variable name.The variable name
nowcould be more descriptive to indicate that it represents ticks.Consider this change:
- var now = DateTime.UtcNow.Ticks; + var nowTicks = DateTime.UtcNow.Ticks;And update the usage:
- if ((long)entry.Value < now) + if ((long)entry.Value < nowTicks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs (1)
22-27: Consider reducing sleep duration and adding error handling.The current implementation has two potential issues:
- The 1-minute sleep might cause the system to miss subsequent button presses
- No error handling for button press detection failures
- Thread.Sleep(1000); - if (buttonPressed) - { - Debug.WriteLine("Button pressed. Sleeping one minute"); - Thread.Sleep(TimeSpan.FromMinutes(1)); - } + try + { + Thread.Sleep(1000); + if (buttonPressed) + { + Debug.WriteLine("Button pressed. Starting demo..."); + Thread.Sleep(TimeSpan.FromSeconds(5)); + } + } + catch (System.Exception ex) + { + Debug.WriteLine($"Error handling button press: {ex.Message}"); + }src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs (1)
18-43: Add input validation and consider alternative brightness scaling.The constructor needs two improvements:
- Add validation for the gaugeColors array
- Consider using a non-linear brightness scale to improve visibility of dimmer LEDs
public NeoPixelGauge(ushort pixelsCount, Color[] gaugeColors, byte pin) { + if (gaugeColors == null || gaugeColors.Length == 0) + throw new ArgumentException("At least one color must be provided", nameof(gaugeColors)); + _pixels = new NeoPixelStrip(pin, pixelsCount, new Ws2812B()); _colors = new Color[pixelsCount]; var partitionSize = pixelsCount / gaugeColors.Length; var remaining = pixelsCount % gaugeColors.Length; - var brightnessScale = 1f / pixelsCount; + // Use square root for non-linear brightness scaling + var brightnessScale = 1f / System.Math.Sqrt(pixelsCount); var index = 0; for (var i = 0; i < gaugeColors.Length; i++) { var partitionLength = partitionSize; if (i == 0) { partitionLength += remaining; } for (var j = 0; j < partitionLength; j++) { - var brightness = (index + 1) * brightnessScale; + var brightness = System.Math.Sqrt(index + 1) * brightnessScale; var color = ColorConverter.ScaleBrightness(gaugeColors[i], brightness); _colors[index++] = color; } } }src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md (1)
3-3: Format the URL as a proper Markdown link.Convert the bare URL to a properly formatted Markdown link for better readability and to comply with Markdown best practices.
-https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf +[ESP32 S3 Datasheet](https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Resources/nonname-esp32s3-dev-kit-c-1-pinout.jpgis excluded by!**/*.jpg
📒 Files selected for processing (12)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs(2 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md(1 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/RipTide.Nfirmware.nfproj(1 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/packages.config(1 hunks)src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs(3 hunks)src/nf-telemetry-clients/Peripherals/IO.Peripherals/IO.Peripherals.nfproj(1 hunks)src/nf-telemetry-clients/Peripherals/IO.Peripherals/packages.config(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixel.Peripheral.nfproj(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/Properties/AssemblyInfo.cs(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/packages.config(1 hunks)src/nf-telemetry-clients/TelemetryStash.NfClients.sln(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/packages.config
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/Properties/AssemblyInfo.cs
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md
3-3: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (10)
src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs (1)
115-118: LGTM!Good addition of the null check for
_notificationTimer. This prevents a potentialNullReferenceExceptionwhen the timer has been disposed.src/nf-telemetry-clients/Peripherals/IO.Peripherals/IO.Peripherals.nfproj (2)
30-32: LGTM! Hardware-related references align with NeoPixel support.The addition of ESP32 hardware support and system runtime references is necessary for NeoPixel LED control on ESP32 devices.
Also applies to: 39-41
58-60: LGTM! Project reference to Shared.nfproj.The addition of the Shared project reference suggests proper code organization and reuse.
src/nf-telemetry-clients/Peripherals/IO.Peripherals/packages.config (1)
4-4: LGTM! Package addition aligns with project references.The addition of the nanoFramework.Hardware.Esp32 package matches the reference in the project file.
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs (1)
14-20:❓ Verification inconclusive
Verify GPIO pin 8 compatibility with ESP32 S3.
The button initialization looks correct, but we should verify that GPIO pin 8 is suitable for button input on the ESP32 S3 board.
🏁 Script executed:
#!/bin/bash # Search for pin configuration documentation in README rg -i "gpio.*8|pin.*8" README.mdLength of output: 34
Verify ESP32 S3 GPIO Pin 8 Usage
The button initialization code in
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs(lines 14–20) appears technically correct. However, since our initial search of the repository’s README for any reference to GPIO pin 8 produced no output, it’s unclear whether this pin is documented or reserved for another purpose on the ESP32 S3 board.
- Please manually verify that GPIO pin 8 is available for general-purpose input on your specific ESP32 S3 board.
- Consult the ESP32 S3 datasheet or board-specific documentation to ensure that using pin 8 for a button does not conflict with boot functions or reserved peripherals.
src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs (1)
6-9: Clarify power management requirements for NeoPixel operation.The commented code suggests that LDO2 might be needed for NeoPixel operation on the UM FeatherS3 board. Please clarify:
- Is LDO2 required for proper NeoPixel operation?
- If required, why is the code commented out?
src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixel.Peripheral.nfproj (1)
1-56: Project configuration looks good!The project file includes all necessary dependencies and is correctly structured for nanoFramework.
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/RipTide.Nfirmware.nfproj (1)
33-52: Dependencies and project references are properly configured!All required packages for button handling and NeoPixel support are correctly referenced.
src/nf-telemetry-clients/TelemetryStash.NfClients.sln (1)
57-58: LGTM! NeoPixel.Peripheral project properly integrated.The new project has been correctly added to the solution with appropriate build configurations and is properly nested under the Peripherals section.
Also applies to: 159-164, 189-189
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/packages.config (1)
4-7: LGTM! Required dependencies added for NeoPixel and button functionality.The added packages align well with the project requirements:
- Graphics.Core for visual output
- Iot.Device.Button and System.Device.Gpio for button handling
- Runtime.Events for event management
src/nf-telemetry-clients/Peripherals/IO.Peripherals/IO.Peripherals.nfproj
Outdated
Show resolved
Hide resolved
src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs
Show resolved
Hide resolved
mikmog
force-pushed
the
add-periphals
branch
from
270d9a6 to
f7ef3bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs (1)
136-140: Consider improving variable naming for clarity.While the change to use ticks is good, consider renaming
nowtonowTicksto better indicate the type and purpose of the variable.- var now = DateTime.UtcNow.Ticks; + var nowTicks = DateTime.UtcNow.Ticks; - if ((long)entry.Value < now) + if ((long)entry.Value < nowTicks)src/nf-telemetry-clients/Shared/Tests/Client.Services.Benchmarks/FrameworkBenchmark.cs (2)
96-123: LGTM! Good improvements to collection benchmarks.The renamed methods and increased test size provide more meaningful benchmarks. The different HashTable configurations will help in understanding performance characteristics under various scenarios.
Consider adding comments explaining the purpose of different load factors and initial capacities to help other developers understand the benchmark variations.
Also applies to: 136-143
125-162: Consider adding type-specific benchmarks and documenting performance implications.The new array and hashtable access benchmarks are valuable additions. However, consider the following improvements:
- The hashtable access requires casting from
objecttoint, which might affect performance. Consider adding a comment about this overhead.- Consider adding benchmarks with different value types (e.g., strings, custom objects) to understand the impact of type complexity on performance.
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md (3)
3-3: Format the URL as a proper markdown link.The bare URL should be formatted as a markdown link with a descriptive text.
Apply this diff to improve the formatting:
-https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf +[ESP32-S3 Datasheet](https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
3-3: Format the URL as a proper Markdown link.The bare URL should be formatted as a Markdown link for better readability.
Apply this diff to format the URL:
-https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf +[ESP32-S3 Datasheet](https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
3-4: Format the URL as a proper Markdown link.The bare URL should be formatted as a Markdown link with descriptive text.
Apply this diff to improve the formatting:
-https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf +[ESP32-S3 Datasheet](https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Resources/nonname-esp32s3-dev-kit-c-1-pinout.jpgis excluded by!**/*.jpg
📒 Files selected for processing (13)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs(2 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md(1 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/RipTide.Nfirmware.nfproj(1 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/packages.config(1 hunks)src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs(3 hunks)src/nf-telemetry-clients/Peripherals/IO.Peripherals/IO.Peripherals.nfproj(1 hunks)src/nf-telemetry-clients/Peripherals/IO.Peripherals/packages.config(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixel.Peripheral.nfproj(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/Properties/AssemblyInfo.cs(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/packages.config(1 hunks)src/nf-telemetry-clients/Shared/Tests/Client.Services.Benchmarks/FrameworkBenchmark.cs(3 hunks)src/nf-telemetry-clients/TelemetryStash.NfClients.sln(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/Properties/AssemblyInfo.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixel.Peripheral.nfproj
- src/nf-telemetry-clients/Peripherals/IO.Peripherals/packages.config
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/packages.config
- src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs
- src/nf-telemetry-clients/Clients/RipTide.Nfirmware/packages.config
- src/nf-telemetry-clients/Peripherals/IO.Peripherals/IO.Peripherals.nfproj
- src/nf-telemetry-clients/TelemetryStash.NfClients.sln
👮 Files not reviewed due to content moderation or server errors (2)
- src/nf-telemetry-clients/Clients/RipTide.Nfirmware/RipTide.Nfirmware.nfproj
- src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md
3-3: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs (2)
77-81: Great optimization using ticks instead of DateTime!Using ticks for timestamp comparison and storage is more efficient in terms of memory and performance, which is particularly important for nanoFramework's resource-constrained environment.
115-118: Good defensive programming with null check!The null check for
_notificationTimerprevents potential NullReferenceException when the timer has been disposed, making the code more robust.src/nf-telemetry-clients/Shared/Tests/Client.Services.Benchmarks/FrameworkBenchmark.cs (2)
13-29: LGTM! Good improvements to the benchmark setup.The introduction of the
Thousandconstant and pre-initialized data structures improves code readability and ensures consistent test data across benchmarks.
67-67: LGTM! Good use of the discard operator.The use of the discard operator
_is appropriate for benchmarking scenarios where we're only interested in the performance of the operation, not the result.Also applies to: 73-83
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/RipTide.Nfirmware.nfproj (4)
33-47:✅ Verification successful
Verify the compatibility of package versions.
Please ensure that the versions of the newly added packages are compatible with each other and with the existing dependencies.
🌐 Web query:
What are the latest versions of nanoFramework packages for: - Iot.Device.Button - nanoFramework.Graphics.Core - nanoFramework.Runtime.Events - System.Device.Gpio💡 Result:
The latest versions of the specified .NET nanoFramework packages as of February 2025 are:
Iot.Device.Button
- Version 1.2.775 [1][10]
nanoFramework.Graphics.Core
- Version 1.2.35 (from GitHub release notes) [2]
nanoFramework.Runtime.Events
- Version 1.11.29 (NuGet package) [1][7]
System.Device.Gpio
- Version 1.1.53 (NuGet package with over 720k downloads) [1]
These versions were all updated within the first week of February 2025 according to their respective NuGet and GitHub release histories[1][2][7]. The Iot.Device.Button package shows continued development with frequent updates to support embedded IoT scenarios[10].
Citations:
- 1: https://www.nuget.org/profiles/nanoframework
- 2: https://github.com/nanoframework/lib-nanoFramework.Graphics.Wpf/releases
- 3: https://docs.nanoframework.net/api/nanoFramework.Runtime.Events.html
- 4: https://docs.nanoframework.net/content/architecture/class-libraries.html
- 5: https://docs.nanoframework.net/content/getting-started-guides/virtual-device.html
- 6: https://docs.nanoframework.net/content/getting-started-guides/getting-started-managed.html
- 7: https://www.nuget.org/packages/nanoFramework.Runtime.Events
- 8: https://docs.nanoframework.net/content/getting-started-guides/dotnet-iot-core-vs-nanoframework.html
- 9: https://github.com/nanoframework/nanoFramework.IoT.Device
- 10: https://nanoframework.net
Package Version Compatibility Verified
All referenced package versions in the project file match the latest releases for February 2025:
- Iot.Device.Button: 1.2.775
- nanoFramework.Graphics.Core: 1.2.35
- nanoFramework.Runtime.Events: 1.11.29
- System.Device.Gpio: 1.1.53
No compatibility issues were detected with these versions relative to each other or the existing dependencies.
33-47:✅ Verification successful
Verify package versions for security and compatibility.
The new references look good, but let's verify that we're using the latest secure versions of these packages.
🌐 Web query:
What are the latest versions of the following nanoFramework packages: - nanoFramework.Iot.Device.Button - nanoFramework.Graphics.Core - nanoFramework.Runtime.Events - nanoFramework.System.Device.Gpio💡 Result:
The latest versions of the specified nanoFramework packages as of February 2025 are:
nanoFramework.Iot.Device.Button: Version 1.2.775 (updated February 4, 2025)[4]
nanoFramework.Graphics.Core: Version 1.2.35 (latest release includes updated dependencies)[2][6]
nanoFramework.Runtime.Events: Version 1.11.29 (listed as dependency requirement in System.Device.Gpio package)[3]
nanoFramework.System.Device.Gpio: Version 1.1.53 (updated February 4, 2025)[3]
These versions reflect recent updates to IoT device bindings[4], graphics libraries[6], and core system components[3]. The Runtime.Events package version is derived from its use as a dependency in other updated packages[3].
Citations:
- 1: https://www.nuget.org/packages/nanoFramework.Tools.FirmwareFlasher/2.5.122
- 2: https://github.com/nanoframework/lib-nanoFramework.Graphics.Wpf/releases
- 3: https://www.nuget.org/packages/nanoFramework.System.Device.Gpio
- 4: https://www.nuget.org/packages/nanoFramework.Iot.Device.Button
- 5: https://github.com/nanoframework/nanoFirmwareFlasher/releases
- 6: https://www.nuget.org/packages/nanoFramework.Graphics.Core/
- 7: https://www.nuget.org/packages/System.Device.Gpio/
- 8: https://github.com/nanoframework/nanoFramework.IoT.Device
- 9: https://github.com/nanoframework/nanoFramework.Hosting/releases
- 10: https://github.com/nanoframework/nanoFramework.Graphics/blob/main/nanoFramework.Graphics.Core.nuspec
Verified: Package Versions are Up-to-date
The web query confirms that the package versions referenced in this project are the latest secure releases as of February 2025:
- nanoFramework.Iot.Device.Button: 1.2.775
- nanoFramework.Graphics.Core: 1.2.35
- nanoFramework.Runtime.Events: 1.11.29
- nanoFramework.System.Device.Gpio: 1.1.53
No further updates are necessary for these references.
33-35: LGTM! New references added for button and GPIO functionality.The added references are correctly configured with appropriate versions and hint paths.
Also applies to: 39-41, 42-44, 45-47
49-52: LGTM! Project references added for NeoPixel and Client Services.The project references are correctly configured.
mikmog
force-pushed
the
add-periphals
branch
from
f7ef3bd to
1a49316
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md (1)
3-3: Improve Markdown URL formatting.Static analysis flagged a bare URL (MD034). To enhance readability and follow markdown best practices, consider converting the URL into a markdown link. For example, you could change the line to:
[ESP32 S3 Datasheet](https://www.espressif.com/sites/default/files/documentation/esp32-s3_datasheet_en.pdf)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
src/nf-telemetry-clients/Shared/Tests/Client.Services.Benchmarks/FrameworkBenchmark.cs (2)
32-93: Ensure consistent use of discard operator across string concatenation benchmarks.Some methods use the discard operator (_) while others use named variables (text). For consistent benchmarking and to prevent compiler optimizations, consider using the discard operator consistently.
Apply this diff to the remaining methods:
public void ConcatString_Plus() { - string text = null; foreach (var word in _hundredWords) { - text += word; + var _ = word; // Prevent optimization while maintaining operation cost } } public void ConcatString_Interpolation() { - string text = null; foreach (var word in _hundredWords) { - text += $" {word} "; + var _ = $" {word} "; } } public void ConcatString_StringConcatForEach() { - string text = null; foreach(var word in _hundredWords) { - text = string.Concat(text, word); + var _ = string.Concat(null, word); } }
135-143: Optimize ArrayList benchmark with initial capacity.The ArrayList benchmark could be improved by testing with initial capacity, similar to the HashTable benchmarks.
Apply this diff:
public void ArrayList_Add_thousand() { - var al = new ArrayList(); + var al = new ArrayList(Thousand); // Pre-allocate capacity for better performance comparison for (var i = 0; i < Thousand; i++) { al.Add(i); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Resources/nonname-esp32s3-dev-kit-c-1-pinout.jpgis excluded by!**/*.jpg
📒 Files selected for processing (13)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs(2 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md(1 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/RipTide.Nfirmware.nfproj(1 hunks)src/nf-telemetry-clients/Clients/RipTide.Nfirmware/packages.config(1 hunks)src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs(3 hunks)src/nf-telemetry-clients/Peripherals/IO.Peripherals/IO.Peripherals.nfproj(1 hunks)src/nf-telemetry-clients/Peripherals/IO.Peripherals/packages.config(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixel.Peripheral.nfproj(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/Properties/AssemblyInfo.cs(1 hunks)src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/packages.config(1 hunks)src/nf-telemetry-clients/Shared/Tests/Client.Services.Benchmarks/FrameworkBenchmark.cs(3 hunks)src/nf-telemetry-clients/TelemetryStash.NfClients.sln(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/nf-telemetry-clients/Peripherals/IO.Peripherals/packages.config
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixel.Peripheral.nfproj
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/NeoPixelGauge.cs
- src/nf-telemetry-clients/Peripherals/IO.Peripherals/IO.Peripherals.nfproj
- src/nf-telemetry-clients/Clients/RipTide.Nfirmware/Program.cs
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/packages.config
- src/nf-telemetry-clients/Clients/RipTide.Nfirmware/RipTide.Nfirmware.nfproj
- src/nf-telemetry-clients/Peripherals/Bluetooth.Sensor/BluetoothSensor.cs
- src/nf-telemetry-clients/Clients/RipTide.Nfirmware/packages.config
- src/nf-telemetry-clients/TelemetryStash.NfClients.sln
- src/nf-telemetry-clients/Peripherals/NeoPixel.Peripheral/Properties/AssemblyInfo.cs
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
src/nf-telemetry-clients/Clients/RipTide.Nfirmware/README.md
3-3: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/nf-telemetry-clients/Shared/Tests/Client.Services.Benchmarks/FrameworkBenchmark.cs (3)
13-30: LGTM! Well-structured setup for benchmarking.The setup is well-designed with:
- Use of constant for size improves maintainability
- Pre-initialization of collections ensures accurate benchmarking
- Identical data population enables fair comparison
95-123: LGTM! Well-structured HashTable benchmarks.The HashTable benchmarks provide good coverage of different scenarios:
- Basic addition
- Maximum load factor
- Initial capacity with maximum load factor
146-162: LGTM! Good addition of retrieval benchmarks.The new Array_Get_by_index and HashTable_Get_by_index benchmarks provide valuable performance comparison for retrieval operations.
Summary by CodeRabbit
New Features
Documentation
Chores