nrf52840/feather_diy Enhancements - #11357
Conversation
Revert a3eced5 (meshtastic#1894): variant.h should specify Arduino pin names, not MCU GPIO numbers - they are mapped by ~/.platformio/packages/framework-arduinoadafruitnrf52/libraries/Wire/Wire_nRF52.cpp through the g_ADigitalPinMap table from ~/.platformio/packages/framework-arduinoadafruitnrf52/variants/feather_nrf52840_express/variant.cpp. Closes meshtastic#9250
None of LED_RED, LED_GREEN, LED_BLUE are used in the code (only PIN_LED{1|2|3} are), so there is no need to name the LEDs wrongly.
On RevD the NeoPixel is on the 3V3 rail along with the MCU and thus cannot be turned off. If we turn it off from the beginning, it does not need to be turned off again in doDeepSleep(), so there is no longer a reason to misuse PIN_3V3_EN for this. The Feather does not have a separately switchable 3V3 rail, so better keep that undefined. (I guess we could set PIN_3V3_EN to an unused exposed pin so users can connect their own external 3V3 switcher if they want, but I can't find a suitable variant-specific hook to turn it on then - lateInitVariant() is too late, it must come before I2C scanning, and initVariant() as used by other variants is already used internally by the Adafruit nRF52 framework. Why is that done in variant-specific code anyway, couldn't setup() do that generically, just as it does for PIN_POWER_EN? Would that be too late already on some boards? If not, why are there separate PIN_3V3_EN and PIN_POWER_EN, could they be unified? The only other difference I see is that PIN_POWER_EN is open-drain and PIN_3V3_EN is push-pull.)
Using the built-in ADC connection. There is no built-in way to detect USB power or charging status.
📝 WalkthroughWalkthroughThe Feather DIY variant now uses direct Arduino pin numbers for board peripherals, corrects LED mappings, defines battery and ADC settings, and disables NeoPixel power during late initialization. ChangesFeather DIY hardware configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
variants/nrf52840/feather_diy/variant.cpp (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce this comment block to two lines.
Keep the RevD versus RevE rationale. State the inherited
pinMode()setup in the same concise block.As per coding guidelines, “Keep comments minimal—normally one or two lines—and document only non-obvious rationale.”
🤖 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 `@variants/nrf52840/feather_diy/variant.cpp` around lines 28 - 32, Condense the comment block above the NeoPixel power-off logic to two lines while preserving the RevD versus RevE rationale and noting that initVariant() already sets pinMode().Source: Coding guidelines
🤖 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 `@variants/nrf52840/feather_diy/variant.h`:
- Line 47: Correct the physical-pin annotation on PIN_WIRE_SCL using the Feather
g_ADigitalPinMap, ensuring it identifies the actual SCL pin and differs from the
PIN_WIRE_SDA annotation while leaving the logical pin value unchanged.
---
Nitpick comments:
In `@variants/nrf52840/feather_diy/variant.cpp`:
- Around line 28-32: Condense the comment block above the NeoPixel power-off
logic to two lines while preserving the RevD versus RevE rationale and noting
that initVariant() already sets pinMode().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8aaba38-7bf6-4c11-8b7b-abca7a39cd92
📒 Files selected for processing (2)
variants/nrf52840/feather_diy/variant.cppvariants/nrf52840/feather_diy/variant.h
| #define PIN_WIRE_SDA (0 + 12) // P0.12 22 | ||
| #define PIN_WIRE_SCL (0 + 11) // P0.12 23 | ||
| #define PIN_WIRE_SDA 22 // P0.12 22 | ||
| #define PIN_WIRE_SCL 23 // P0.12 23 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the PIN_WIRE_SCL physical-pin annotation.
PIN_WIRE_SDA and PIN_WIRE_SCL cannot both map to P0.12. Verify this value against the Feather g_ADigitalPinMap and correct the annotation. Incorrect wiring comments can cause a DIY board to connect I2C SCL to the wrong pin.
🤖 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 `@variants/nrf52840/feather_diy/variant.h` at line 47, Correct the physical-pin
annotation on PIN_WIRE_SCL using the Feather g_ADigitalPinMap, ensuring it
identifies the actual SCL pin and differs from the PIN_WIRE_SDA annotation while
leaving the logical pin value unchanged.
There was a problem hiding this comment.
That does not come from me but from a3eced5, but I can add another commit that fixes it if the maintainers prefer.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'variant\.(cpp|h)$' variants | while IFS= read -r file; do
if rg -n -C 3 '\bg_ADigitalPinMap\b|P0\.11|P0\.12' "$file" >/dev/null; then
printf '\n--- %s ---\n' "$file"
rg -n -C 3 '\bg_ADigitalPinMap\b|P0\.11|P0\.12' "$file"
fi
doneLength of output: 23442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Feather DIY variant files ---'
fd -t f . variants/nrf52840/feather_diy -x sh -c 'printf "%s\n" "$1"' sh {}
printf '%s\n' '--- Cited commit: I2C definitions ---'
git show --format=fuller --no-ext-diff a3eced53bbd61ad9e2da90767e1bd8835f53c988 -- variants/nrf52840/feather_diy/variant.h
printf '%s\n' '--- References to Feather Arduino pin 23 / P0.11 ---'
rg -n -C 3 'feather_diy|PIN_WIRE_SCL.*23|P0\.11.*23|23.*P0\.11' \
variants platformio.ini boards.txt 2>/dev/null || trueLength of output: 2511
🤖 web_search executed:
Adafruit Feather nRF52840 Express variant.cpp g_ADigitalPinMap pin 22 pin 23 P0.12 P0.11 GitHub
Length of output: 7617
@cwalther, thank you for the context. The issue is pre-existing, but it remains valid in the current file.
The upstream Feather mapping defines D23 as P0.11 for SCL. The logical value 23 is already correct. Only the annotation needs correction:
`#define` PIN_WIRE_SCL 23 // P0.11 23This is documentation-only and does not change runtime behavior. A small follow-up commit in this PR is appropriate if the maintainers want the corrected pin map. Upstream mapping
You are interacting with an AI system.
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
caveman99
left a comment
caveman99
left a comment
There was a problem hiding this comment.
This changes a lot of pins around. i can not imagine that this variant was horribly broken for so long. I'd like to have some more insight into thes changes.
| #define PIN_WIRE_SDA (0 + 12) // P0.12 22 | ||
| #define PIN_WIRE_SCL (0 + 11) // P0.12 23 | ||
| #define PIN_WIRE_SDA 22 // P0.12 22 | ||
| #define PIN_WIRE_SCL 23 // P0.12 23 |
There was a problem hiding this comment.
Is this true for all revisons? Don't wanna break existing boards
There was a problem hiding this comment.
Yes, at least as far as I can tell from the Rev D and Rev E schematics at https://learn.adafruit.com/introducing-the-adafruit-nrf52840-feather/downloads as well as https://learn.adafruit.com/introducing-the-adafruit-nrf52840-feather/pinouts.
I can not imagine that this variant was horribly broken for so long.
I am as surprised as you are, but nobody has spoken up in #9250, and I have not gotten any response from @thebentern (committer) or @markbirss (apparently original author) on why the change in #1894 was done to begin with. It just seems like nobody except me is using these boards?
2 participants
Some fixes and cleanups to make my self-built node based on an Adafruit Feather nRF52840 Express work and be useful:
See commit messages for details.
I have had this in use for several months on several firmware versions (up to 2.7.26). Note that I only have RevD boards, so I have not been able to test whether the NeoPixel change has the desired effect on RevE, only that it doesn’t break anything on RevD.
🤝 Attestations
Summary by CodeRabbit