feat: impl simple gpio pin allocator#288
Conversation
GPIOAllocator is to become in charge of allocating leases for GPIO Pins to any components that needs it
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR adds a centralized RAII GPIO allocator with ownership tracking, integrates leases into NFC, Ethernet, and hardware control, validates pin conflicts in web configuration, and removes duplicated Ethernet–NFC pin intersection handling. ChangesGPIO leasing integration
Sequence Diagram(s)sequenceDiagram
participant main
participant NfcManager
participant HomeKitLock
participant GPIOAllocator
participant Ethernet
main->>NfcManager: construct with NFC pin configuration
NfcManager->>GPIOAllocator: acquire NFC pins
GPIOAllocator-->>NfcManager: return leases or errors
main->>HomeKitLock: initializeETH()
HomeKitLock->>GPIOAllocator: acquire Ethernet pins
GPIOAllocator-->>HomeKitLock: return leases or errors
HomeKitLock->>Ethernet: initialize Ethernet
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🧹 Nitpick comments (1)
main/HomeKitLock.cpp (1)
493-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
value_orand no output on miss.Inside
if(s.has_value()),s.value_or("Not allocated")can never yield the fallback; and when the pin is unowned nothing is logged, so the command silently does nothing.♻️ Optional cleanup
new SpanUserCommand('G', "Who owns this GPIO Pin?", [](const char* c) { uint8_t i = atoi(c+1); auto s = GPIOAllocator::instance().owner_of(i); - if(s.has_value()){ - ESP_LOGI(TAG, "Owner: %s", s.value_or("Not allocated").c_str()); - } + ESP_LOGI(TAG, "Owner: %s", s.has_value() ? s->c_str() : "Not allocated"); });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4286c5c-2a01-4a97-987e-276584e840c6
📒 Files selected for processing (10)
main/CMakeLists.txtmain/GPIOAllocator.cppmain/HardwareManager.cppmain/HomeKitLock.cppmain/NfcManager.cppmain/WebServerManager.cppmain/include/GPIOAllocator.hppmain/include/HardwareManager.hppmain/include/NfcManager.hppmain/main.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
assess the relevance of the changes to the purpose of the PR and do not mention anything if they are relevant.
Files:
main/CMakeLists.txtmain/GPIOAllocator.cppmain/NfcManager.cppmain/include/HardwareManager.hppmain/include/NfcManager.hppmain/include/GPIOAllocator.hppmain/WebServerManager.cppmain/HomeKitLock.cppmain/main.cppmain/HardwareManager.cpp
🔇 Additional comments (12)
main/include/GPIOAllocator.hpp (1)
25-65: LGTM!main/GPIOAllocator.cpp (1)
1-3: LGTM!main/CMakeLists.txt (1)
4-4: LGTM!main/include/NfcManager.hpp (1)
98-107: LGTM!main/NfcManager.cpp (1)
230-237: LGTM!main/HomeKitLock.cpp (1)
130-302: LGTM!main/main.cpp (1)
133-142: LGTM!main/WebServerManager.cpp (3)
5-5: LGTM!
885-894: LGTM!
1098-1109: 🎯 Functional CorrectnessThis rejection doesn’t affect the normal config save flow. The UI posts
diff(...)payloads for/config/save, so unchanged/self-owned pins are omitted and won’t reach thisowner_of(...)check. If there’s another caller sending full objects to this endpoint, that would be a separate case.> Likely an incorrect or invalid review comment.main/include/HardwareManager.hpp (1)
8-10: LGTM!Also applies to: 145-157
main/HardwareManager.cpp (1)
5-9: LGTM!Also applies to: 36-56, 208-227, 393-416, 466-466, 530-532, 580-614
| if (meta->func == PinFunctions::ALT_ACTION_INIT) { | ||
| new_lease.value().set_pullup(true); | ||
| gpio_set_intr_type(gpio_num_t(s.newValue), GPIO_INTR_NEGEDGE); | ||
| gpio_isr_handler_add(gpio_num_t(s.newValue), initiator_isr_handler, (void*)this); | ||
| } | ||
|
|
||
| alloc_entry = std::move(new_lease); | ||
| ESP_LOGI(TAG, "Acquired pin %d for %s", s.newValue, meta->tag); | ||
| } else { | ||
| ESP_LOGE(TAG, "Failed to acquire pin %d, error: %d", s.newValue, new_lease.error()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect initiator ISR handler + queue usage and any ISR-service install outside begin()
fd -t f 'HardwareManager.cpp' main | xargs -I{} sed -n '1,240p;450,480p' {}
rg -nP 'gpio_install_isr_service|initiator_isr_handler|m_initiatorQueue' main/HardwareManager.cppRepository: rednblkx/HomeKey-ESP32
Length of output: 12815
Install the ISR service before dynamically adding ALT_ACTION_INIT
main/HardwareManager.cpp:145-155 can call gpio_isr_handler_add(...) during HW_CONFIG_CHANGED even when begin() never installed the ISR service or created m_initiatorQueue/initiator_task (for example, if hkAltActionInitPin was 255 at boot). Lazily initialize that setup here, or guard the dynamic-add path until it exists.
1 participant
GPIOAllocator is to be in charge of allocating leases for GPIO Pins to any components that needs controlling any of the pins, at the same time maintaining a central repository of who owns what pin to help avoid conflicts and henceforth becomes critical that anytime a pin is assigned, it should be first of all acquired from the allocator.
LIfecycle:
Summary by CodeRabbit