Skip to content

Add a pn532_uart component to support the HSU interface#30

Open
chbmuc wants to merge 11 commits into
rednblkx:mainfrom
chbmuc:hsu
Open

Add a pn532_uart component to support the HSU interface#30
chbmuc wants to merge 11 commits into
rednblkx:mainfrom
chbmuc:hsu

Conversation

@chbmuc
Copy link
Copy Markdown

@chbmuc chbmuc commented Jan 15, 2025

I had to use the serial interface, to connect the PN532 module. Since my cable is approximately 5 meters long and consists of only a few wires, using the SPI interface was not an option.

This pull request includes a new component for the HSU interface, a small patch to make the RF behavior configurable, and some updates to the README.

While I still recommend using the SPI interface whenever possible, the UART component now seems to work reliably (with keep_rf_on: true). I hope this solution proves helpful to others facing similar limitations with their reader setup.

Summary by CodeRabbit

  • Documentation

    • README updated to document PN532 support over SPI and UART, with examples and guidance for HomeKey and RF handling.
  • New Features

    • Added UART-based PN532 option alongside existing SPI support.
    • Added configurable option to keep the NFC RF field on.
  • Improvements

    • Improved RF management and wakeup reliability for PN532 devices.
    • More flexible NFC configuration and clearer HomeKey setup guidance.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jan 15, 2025

Walkthrough

Adds UART support for the PN532 NFC module and a keep_rf_on configuration option. Updates README to document pn532_uart alongside pn532_spi, modifies PN532 core to conditionally manage RF and wakeup, and introduces a new pn532_uart component with UART I/O and framing logic.

Changes

Cohort / File(s) Summary
Documentation
README.md
Documented HomeKey support for both pn532_spi and pn532_uart, added example configs (SPI and UART), and expanded guidance on keep_rf_on and RF handling.
PN532 core
components/pn532/__init__.py, components/pn532/pn532.h, components/pn532/pn532.cpp
Added CONF_KEEP_RF_ON and keep_rf_on_ with setter; new flush_rx_() virtual method; changed setup to perform an initial SAM wakeup handshake, conditional RF-off behavior based on keep_rf_on_, updated powerdown bits, and added RX flush before ECP processing.
PN532 UART component
components/pn532_uart/__init__.py, components/pn532_uart/pn532_uart.h, components/pn532_uart/pn532_uart.cpp
New pn532_uart component: config/schema, PN532Uart class, UART setup, read/write methods, response parsing (normal/extended frames), checksum validation, flush_rx_() override, and verbose logging.

Sequence Diagram

sequenceDiagram
    participant User as User Config
    participant ESP as ESPHome
    participant PN as PN532 (UART)

    User->>ESP: Configure pn532_uart + keep_rf_on
    ESP->>PN: Open UART, send WAKEUP_REQUEST
    PN-->>ESP: ACK / wakeup response
    ESP->>PN: PN532 COMMAND_SAMCONFIGURATION (setup)
    PN-->>ESP: SAM response
    loop NFC operation
      ESP->>PN: write_data (command/frame)
      alt response available
        PN-->>ESP: data ready
        ESP->>PN: read_response (validate/frame)
      end
    end
Loading

Poem

🐰
UART hums, a tiny hop and click,
PN532 wakes—my whiskers twitch!
Fields held gently, never gone,
HomeKey opens with a dawn-time song.
A rabbit cheers, circuits snug and quick.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a new pn532_uart component to support the HSU (UART) interface, which is the primary feature introduced in this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chbmuc chbmuc mentioned this pull request Jan 15, 2025
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (6)
components/pn532/__init__.py (1)

13-13: Consider documenting the keep_rf_on option.

The keep_rf_on option is crucial for HomeKey functionality. Consider adding a comment explaining its purpose and when it should be enabled.

 CONF_KEEP_RF_ON = "keep_rf_on"
+# Controls whether RF field remains active between operations.
+# Enable this for better HomeKey functionality with UART interface.

Also applies to: 46-46

components/pn532_uart/pn532_uart.cpp (2)

62-65: Document and validate timing workaround.

The 3ms delay workaround needs documentation explaining why it's needed and whether it's sufficient for all use cases.

   // Workaround for timing issues
+  // The PN532 might need additional time to prepare the response
+  // 3ms delay based on empirical testing with HomeKey
   if(!this->available()) {
       delay(3);
   }

154-154: Fix namespace in closing comment.

The namespace in the closing comment is incorrect.

-}  // namespace pn532_spi
+}  // namespace pn532_uart
README.md (3)

61-62: Fix grammatical issues in the UART component description.

Add missing commas and fix typos:

-There is also a `pn532_uart` component that can be used when the PN532 module needs to be connected via the HSU interface. However it is recomended to use the SPI interface whenever posible, as it is more reliable.
+There is also a `pn532_uart` component that can be used when the PN532 module needs to be connected via the HSU interface. However, it is recommended to use the SPI interface whenever possible, as it is more reliable.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~61-~61: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ... to be connected via the HSU interface. However it is recomended to use the SPI interfa...

(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)


244-244: Consider rephrasing for conciseness.

Replace "at the moment" with "currently" for better readability:

-Only PN532 over SPI(`pn532_spi` component) or the PN532 over HSU(`pn532_uart` component) are supported at the moment due to required modifications that haven't been ported to other protocols or chips
+Only PN532 over SPI(`pn532_spi` component) or the PN532 over HSU(`pn532_uart` component) are currently supported due to required modifications that haven't been ported to other protocols or chips
🧰 Tools
🪛 LanguageTool

[style] ~244-~244: For conciseness, consider replacing this expression with an adverb.
Context: ...U(pn532_uart component) are supported at the moment due to required modifications that have...

(AT_THE_MOMENT)


248-249: Fix typos and grammatical issues in the RF note.

Add missing comma and fix typos:

-> Esphome is turning of the RF side of the PN532 module whenever posible to avoid wifi interference. However this is causing problems with Homekey. You have to set the `keep_rf_on` option for reliable Homekey communication.
+> ESPHome is turning off the RF side of the PN532 module whenever possible to avoid Wi-Fi interference. However, this is causing problems with HomeKey. You have to set the `keep_rf_on` option for reliable HomeKey communication.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~248-~248: “of” seems less likely than “off” (as in: go/turn off, off the coast).
Context: ...terval` over 500ms > Esphome is turning of the RF side of the PN532 module wheneve...

(AI_HYDRA_LEO_CP_OF_OFF)


[uncategorized] ~248-~248: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...ver posible to avoid wifi interference. However this is causing problems with Homekey. ...

(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b5085d and c541734.

📒 Files selected for processing (7)
  • README.md (5 hunks)
  • components/pn532/__init__.py (3 hunks)
  • components/pn532/pn532.cpp (4 hunks)
  • components/pn532/pn532.h (2 hunks)
  • components/pn532_uart/__init__.py (1 hunks)
  • components/pn532_uart/pn532_uart.cpp (1 hunks)
  • components/pn532_uart/pn532_uart.h (1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md

[uncategorized] ~61-~61: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ... to be connected via the HSU interface. However it is recomended to use the SPI interfa...

(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)


[style] ~244-~244: For conciseness, consider replacing this expression with an adverb.
Context: ...U(pn532_uart component) are supported at the moment due to required modifications that have...

(AT_THE_MOMENT)


[uncategorized] ~248-~248: “of” seems less likely than “off” (as in: go/turn off, off the coast).
Context: ...terval` over 500ms > Esphome is turning of the RF side of the PN532 module wheneve...

(AI_HYDRA_LEO_CP_OF_OFF)


[uncategorized] ~248-~248: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...ver posible to avoid wifi interference. However this is causing problems with Homekey. ...

(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)

🪛 Markdownlint (0.37.0)
README.md

245-245: null
Blank line inside blockquote

(MD028, no-blanks-blockquote)

🔇 Additional comments (5)
components/pn532_uart/pn532_uart.h (1)

12-24: Well-structured class definition!

The PN532Uart class properly inherits from both PN532 and UARTDevice, with clear method declarations and appropriate access modifiers.

components/pn532_uart/__init__.py (1)

22-29: Correct UART configuration validation!

The UART settings (8N1) match the PN532's HSU interface requirements as per the datasheet.

components/pn532/pn532.h (1)

57-58: LGTM! The RF management enhancement looks good.

The addition of the keep_rf_on_ member variable and its setter method is well-implemented. The default value of false maintains backward compatibility.

Also applies to: 110-110

components/pn532/pn532.cpp (1)

105-107: LGTM! The RF management implementation is consistent.

The conditional checks for keep_rf_on_ are correctly added at all relevant locations where turn_off_rf_() is called. This ensures consistent RF management behavior throughout the component's lifecycle.

Also applies to: 250-252, 267-269, 355-357

README.md (1)

270-288: LGTM! The UART configuration example is clear and well-structured.

The example demonstrates the correct setup for the HSU interface with appropriate pin configuration and baud rate settings.

Comment thread components/pn532_uart/pn532_uart.cpp
Comment thread components/pn532_uart/pn532_uart.cpp
Comment thread components/pn532_uart/pn532_uart.cpp
@chbmuc chbmuc changed the title Add a pn532_uart component to support the HSU interface WIP: Add a pn532_uart component to support the HSU interface Jan 21, 2025
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (3)
components/pn532_uart/pn532_uart.cpp (3)

16-31: 🛠️ Refactor suggestion

Enhance setup reliability with proper timing and error handling.

The setup sequence should follow the PN532 datasheet recommendations for timing and include error handling for failed wakeup attempts.

 void PN532Uart::setup() {
   uint8_t buf;
 
   ESP_LOGI(TAG, "PN532Uart setup started!");
 
   this->dump_config();
 
   ESP_LOGI(TAG, "initiating wakeup...");
   // wakeup
   this->write_array(WAKEUP_REQUEST, sizeof(WAKEUP_REQUEST));
-  while (this->available()) {
-    this->read_byte(&buf);
+  delay(5);  // Minimum 5ms delay recommended by datasheet
+  
+  // Verify wakeup success
+  uint32_t start = millis();
+  bool wakeup_success = false;
+  while (this->available() && (millis() - start) < 100) {
+    this->read_byte(&buf);
+    wakeup_success = true;
   }
+  if (!wakeup_success) {
+    ESP_LOGW(TAG, "PN532 wakeup failed!");
+  }
 
   PN532::setup();
 }

37-42: 🛠️ Refactor suggestion

Add write verification for reliability.

The write_data method should verify if the write operation was successful.


59-68: ⚠️ Potential issue

Add bounds checking for header array access.

The code reads 7 bytes without verifying if enough data is available.

🧹 Nitpick comments (1)
components/pn532_uart/pn532_uart.cpp (1)

140-142: Enhance configuration dump with UART-specific details.

Consider adding UART-specific configuration details (baud rate, pins, etc.) to the dump_config output for better debugging.

 void PN532Uart::dump_config() {
   PN532::dump_config();
+  ESP_LOGCONFIG(TAG, "  UART:");
+  ESP_LOGCONFIG(TAG, "    Baud Rate: %d", this->get_baud_rate());
+  ESP_LOGCONFIG(TAG, "    TX Pin: %d", this->get_tx_pin());
+  ESP_LOGCONFIG(TAG, "    RX Pin: %d", this->get_rx_pin());
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c541734 and aa7b86f.

📒 Files selected for processing (2)
  • components/pn532/pn532.cpp (5 hunks)
  • components/pn532_uart/pn532_uart.cpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • components/pn532/pn532.cpp
🔇 Additional comments (1)
components/pn532_uart/pn532_uart.cpp (1)

4-7: Well-documented implementation with official references!

Good practice including links to official NXP documentation for future maintenance and reference.

Comment on lines +111 to +120
uint8_t checksum = header[5] + header[6]; // TFI + Command response code
if (extended_frame) {
checksum = header[8] + header[9];
}
for (int i = 0; i < len - 1; i++) {
uint8_t dat = data[i];
checksum += dat;
}
checksum = ~checksum + 1;

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Protect against integer overflow in checksum calculation.

The checksum calculation could overflow without proper bounds checking.

-  uint8_t checksum = header[5] + header[6];  // TFI + Command response code
+  uint16_t checksum = 0;  // Use wider type to prevent overflow
+  checksum += header[5];  // TFI
+  checksum += header[6];  // Command response code
   if (extended_frame) {
-    checksum = header[8] + header[9];
+    checksum = 0;
+    checksum += header[8];
+    checksum += header[9];
   }
   for (int i = 0; i < len - 1; i++) {
     uint8_t dat = data[i];
     checksum += dat;
   }
-  checksum = ~checksum + 1;
+  checksum = (~checksum + 1) & 0xFF;  // Ensure 8-bit result
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uint8_t checksum = header[5] + header[6]; // TFI + Command response code
if (extended_frame) {
checksum = header[8] + header[9];
}
for (int i = 0; i < len - 1; i++) {
uint8_t dat = data[i];
checksum += dat;
}
checksum = ~checksum + 1;
uint16_t checksum = 0; // Use wider type to prevent overflow
checksum += header[5]; // TFI
checksum += header[6]; // Command response code
if (extended_frame) {
checksum = 0;
checksum += header[8];
checksum += header[9];
}
for (int i = 0; i < len - 1; i++) {
uint8_t dat = data[i];
checksum += dat;
}
checksum = (~checksum + 1) & 0xFF; // Ensure 8-bit result

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure - looks like a valid suggestion, however the relevant code is the same implementation as in the HSU version

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chbmuc You're absolutely right - I've verified that both the SPI and UART implementations use the same checksum calculation approach. While the overflow protection suggestion is valid, it would be better to address this consistently across all implementations in a separate PR. Would you like me to create an issue to track this potential improvement?

🔗 Analysis chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for HSU implementation files and checksum calculation
rg -l "HSU|hsu" --type cpp
rg "checksum.*=.*header" --type cpp -B 2 -A 4

Length of output: 1354


✏️ Learnings added
Learnt from: chbmuc
PR: rednblkx/HAP-ESPHome#30
File: components/pn532_uart/pn532_uart.cpp:111-120
Timestamp: 2025-01-26T16:21:43.263Z
Learning: The checksum calculation in PN532 implementations (both SPI and UART) follows the same approach using uint8_t type, maintaining consistency across different interface implementations.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread components/pn532_uart/pn532_uart.cpp
@chbmuc chbmuc changed the title WIP: Add a pn532_uart component to support the HSU interface Add a pn532_uart component to support the HSU interface Jan 26, 2025
@rednblkx
Copy link
Copy Markdown
Owner

Hi!

Thanks for the PR, sorry i haven't replied, had some stuff to deal with and haven't been checking the projects.

Everything looks good, great work adapting the driver for uart and thanks for updating the documentation too.

I can't test it right now but the code looks good, i'll try to give it a go in the coming days

Cheers!

@Noschvie
Copy link
Copy Markdown

Any news about that?

@rednblkx rednblkx force-pushed the main branch 6 times, most recently from 4e3bba5 to e37b956 Compare February 21, 2026 15:54
chbmuc and others added 4 commits February 21, 2026 22:29
Add read_ready_(true) calls before reading in UART read_response() and
read_data(), matching the SPI transport behavior. Without this, UART
reads could get garbage data if the PN532 hasn't finished preparing
its response.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
components/pn532/pn532.cpp (1)

84-104: ⚠️ Potential issue | 🟡 Minor

WAKEUP_FAILED reused for RF configuration errors.

Both the write_command_ (line 91) and read_response (line 102) failures inside the RFCONFIGURATION block set error_code_ = WAKEUP_FAILED. A dump_config() call after such a failure will log "Wake Up command failed!" — an incorrect diagnostic for an RF configuration error. Consider using SAM_COMMAND_FAILED as a catch-all setup error, or adding a dedicated RF_CONF_FAILED enum value.

🧹 Nitpick comments (1)
components/pn532_uart/pn532_uart.cpp (1)

35-36: Simplify redundant ternary.

(this->available() > 0) ? true : false is equivalent to the comparison itself.

♻️ Proposed simplification
-  return (this->available() > 0) ? true : false;
+  return this->available() > 0;
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa7b86f and 3722ac2.

📒 Files selected for processing (6)
  • README.md
  • components/pn532/__init__.py
  • components/pn532/pn532.cpp
  • components/pn532/pn532.h
  • components/pn532_uart/pn532_uart.cpp
  • components/pn532_uart/pn532_uart.h
🚧 Files skipped from review as they are similar to previous changes (2)
  • components/pn532/init.py
  • components/pn532/pn532.h
🧰 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:

  • components/pn532_uart/pn532_uart.h
  • components/pn532_uart/pn532_uart.cpp
  • README.md
  • components/pn532/pn532.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: chbmuc
Repo: rednblkx/HAP-ESPHome PR: 30
File: components/pn532_uart/pn532_uart.cpp:0-0
Timestamp: 2025-01-26T16:19:33.707Z
Learning: In the PN532 UART component's setup function, no delay is required after sending the wakeup request.
📚 Learning: 2025-01-26T16:19:33.707Z
Learnt from: chbmuc
Repo: rednblkx/HAP-ESPHome PR: 30
File: components/pn532_uart/pn532_uart.cpp:0-0
Timestamp: 2025-01-26T16:19:33.707Z
Learning: In the PN532 UART component's setup function, no delay is required after sending the wakeup request.

Applied to files:

  • components/pn532_uart/pn532_uart.cpp
  • components/pn532/pn532.cpp
📚 Learning: 2025-01-26T16:21:43.438Z
Learnt from: chbmuc
Repo: rednblkx/HAP-ESPHome PR: 30
File: components/pn532_uart/pn532_uart.cpp:111-120
Timestamp: 2025-01-26T16:21:43.438Z
Learning: The checksum calculation in PN532 implementations (both SPI and UART) follows the same approach using uint8_t type, maintaining consistency across different interface implementations.

Applied to files:

  • components/pn532_uart/pn532_uart.cpp
📚 Learning: 2025-01-26T16:22:16.945Z
Learnt from: chbmuc
Repo: rednblkx/HAP-ESPHome PR: 30
File: components/pn532_uart/pn532_uart.cpp:44-54
Timestamp: 2025-01-26T16:22:16.945Z
Learning: The write_array method in the UART component doesn't return a size value, it's a void function.

Applied to files:

  • components/pn532_uart/pn532_uart.cpp
📚 Learning: 2025-01-26T16:18:56.486Z
Learnt from: chbmuc
Repo: rednblkx/HAP-ESPHome PR: 30
File: components/pn532_uart/pn532_uart.cpp:37-42
Timestamp: 2025-01-26T16:18:56.486Z
Learning: In ESPHome's UART implementation, the write_array method doesn't return the number of bytes written. It's a void method from the UARTDevice base class.

Applied to files:

  • components/pn532_uart/pn532_uart.cpp
🧬 Code graph analysis (1)
components/pn532/pn532.cpp (1)
components/pn532/pn532.h (1)
  • keep_rf_on_ (110-163)
🪛 Clang (14.0.6)
components/pn532_uart/pn532_uart.h

[error] 3-3: 'esphome/core/component.h' file not found

(clang-diagnostic-error)

🪛 LanguageTool
README.md

[grammar] ~59-~59: Ensure spelling is correct
Context: ...ed via the HSU interface. However it is recomended to use the SPI interface whenever posib...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~59-~59: Ensure spelling is correct
Context: ...ended to use the SPI interface whenever posible, as it is more reliable. > [!TIP] > Fo...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[style] ~242-~242: For conciseness, consider replacing this expression with an adverb.
Context: ...U(pn532_uart component) are supported at the moment due to required modifications that have...

(AT_THE_MOMENT)


[grammar] ~246-~246: Ensure spelling is correct
Context: ...he RF side of the PN532 module whenever posible to avoid wifi interference. However thi...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~248-~248: Ensure spelling is correct
Context: ...ion. Example configuration for the SPI inteface: yaml spi: clk_pin: 4 miso_pin: 5 mosi_pin: 6 pn532_spi: id: nfc_spi_module cs_pin: 7 update_interval: 100ms keep_rf_on: true homekit: lock: - id: <insert lock id> nfc_id: nfc_spi_module Alternative example configuration for th...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~268-~268: Ensure spelling is correct
Context: ...ative example configuration for the HSU inteface: yaml uart: id: uart_nfc tx_pin: 17 rx_pin: 16 baud_rate: 115200 pn532_spi: id: nfc_uart_module uart_id: uart_nfc update_interval: 100ms keep_rf_on: true homekit: lock: - id: <insert lock id> nfc_id: nfc_uart_module ## Support & Contributing If you wish to c...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🪛 markdownlint-cli2 (0.21.0)
README.md

[warning] 243-243: Blank line inside blockquote

(MD028, no-blanks-blockquote)

🔇 Additional comments (3)
components/pn532_uart/pn532_uart.h (1)

1-28: LGTM!

Class interface is clean and consistent with the implementation in pn532_uart.cpp. The static analysis error for esphome/core/component.h is a sandbox false positive — the ESPHome build environment supplies these headers.

components/pn532/pn532.cpp (1)

107-109: LGTM — consistent conditional RF control.

The keep_rf_on_ guard here and at the three other call-sites (lines 256–258, 273–275, 362–364) is applied uniformly.

components/pn532_uart/pn532_uart.cpp (1)

158-163: LGTM — flush_rx_ drains stale bytes correctly.

The byte-by-byte drain loop mirrors the setup() and pn532.cpp flush pattern.

Comment on lines +119 to +147
if (full_len == 0)
len = 0;

ESP_LOGV(TAG, "Reading response of length %d", len);

data.resize(len + 1);
this->read_array(data.data(), len + 1);

ESP_LOGV(TAG, "Response data: %s", format_hex_pretty_to(hex_buf, sizeof(hex_buf), data.data(), data.size()));

uint8_t checksum = header[5] + header[6]; // TFI + Command response code
if (extended_frame) {
checksum = header[8] + header[9];
}
for (int i = 0; i < len - 1; i++) {
uint8_t dat = data[i];
checksum += dat;
}
checksum = ~checksum + 1;

if (data[len - 1] != checksum) {
ESP_LOGV(TAG, "read data invalid checksum! %02X != %02X", data[len - 1], checksum);
return false;
}

if (data[len] != 0x00) {
ESP_LOGV(TAG, "read data invalid postamble!");
return false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Undefined behaviour when full_len == 0.

The guard at lines 119–120 correctly assigns len = 0, but the subsequent code still has UB:

  • data[len - 1]: len is uint16_t; 0 - 1 promotes to int(-1), which when used as a std::vector index converts to size_t(UINT_MAX) — an out-of-bounds access.
  • data.erase(data.end() - 2, data.end()): a 1-element vector has no room to erase 2 elements — iterator underflow.

full_len == 0 should not occur with a healthy PN532 (LEN always covers at least TFI+CMD = 2), but the guard is already present precisely for defensive reasons and should be made complete.

🛡️ Proposed defensive fix
  uint16_t len = full_len - 1;
  if (full_len == 0)
    len = 0;

+ if (len < 1) {
+   ESP_LOGV(TAG, "read_response: empty response, discarding");
+   return false;
+ }
+
  ESP_LOGV(TAG, "Reading response of length %d", len);

Comment on lines +21 to +29
if (!this->write_command_({
PN532_COMMAND_SAMCONFIGURATION,
0x01, // normal mode
0xff, // max. timeout
})) {
ESP_LOGE(TAG, "No wakeup ack");
this->mark_failed();
return;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

error_code_ not set on initial wakeup write_command_ failure.

When write_command_ returns false here, mark_failed() is called but error_code_ remains NONE. dump_config() therefore emits no error message for this failure path, making diagnosis harder.

🐛 Proposed fix
  if (!this->write_command_({
          PN532_COMMAND_SAMCONFIGURATION,
          0x01,  // normal mode
          0xff,  // max. timeout
      })) {
    ESP_LOGE(TAG, "No wakeup ack");
+   this->error_code_ = WAKEUP_FAILED;
    this->mark_failed();
    return;
  }

Comment thread README.md

This repository also includes the `pn532` and `pn532_spi` components which are just slightly modified versions of the official ones from the ESPHome repository to suit HomeKey needs with no extra options added to them nor deleted, however, it's not guaranteed to be kept up to date with upstream changes.

There is also a `pn532_uart` component that can be used when the PN532 module needs to be connected via the HSU interface. However it is recomended to use the SPI interface whenever posible, as it is more reliable.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix spelling/grammar errors across changed lines.

Several typos and a grammar error appear in the new text:

Line Issue Fix
59 recomended recommended
59 posible possible
246 Esphome ESPHome
246 turning of the RF turning off the RF
246 posible possible
248 inteface interface
268 inteface interface
📝 Proposed fix
-There is also a `pn532_uart` component that can be used when the PN532 module needs to be connected via the HSU interface. However it is recomended to use the SPI interface whenever posible, as it is more reliable.
+There is also a `pn532_uart` component that can be used when the PN532 module needs to be connected via the HSU interface. However it is recommended to use the SPI interface whenever possible, as it is more reliable.
-> Esphome is turning of the RF side of the PN532 module whenever posible to avoid wifi interference. However this is causing problems with Homekey. You have to set the `keep_rf_on` option for reliable Homekey communication.
+> ESPHome is turning off the RF side of the PN532 module whenever possible to avoid wifi interference. However this is causing problems with Homekey. You have to set the `keep_rf_on` option for reliable Homekey communication.
-Example configuration for the SPI inteface:
+Example configuration for the SPI interface:
-Alternative example configuration for the HSU inteface:
+Alternative example configuration for the HSU interface:

Also applies to: 246-246, 248-248, 268-268

🧰 Tools
🪛 LanguageTool

[grammar] ~59-~59: Ensure spelling is correct
Context: ...ed via the HSU interface. However it is recomended to use the SPI interface whenever posib...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~59-~59: Ensure spelling is correct
Context: ...ended to use the SPI interface whenever posible, as it is more reliable. > [!TIP] > Fo...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

Comment thread README.md
Comment on lines 241 to 243
> [!IMPORTANT]
> Only PN532 over SPI(`pn532_spi` component) is supported at the moment due to required modifications that haven't been ported to other protocols or chips
> Only PN532 over SPI(`pn532_spi` component) or the PN532 over HSU(`pn532_uart` component) are supported at the moment due to required modifications that haven't been ported to other protocols or chips

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Blank line inside blockquote triggers MD028 lint warning.

The blank line between the two > [!IMPORTANT] / > [!NOTE] blocks (line 243) violates markdownlint rule MD028 and may render incorrectly in some Markdown processors.

📝 Proposed fix
 > [!IMPORTANT]
-> Only PN532 over SPI(`pn532_spi` component) or the PN532 over HSU(`pn532_uart` component) are supported at the moment due to required modifications that haven't been ported to other protocols or chips
-
-> [!NOTE]
+> Only PN532 over SPI(`pn532_spi` component) or the PN532 over HSU(`pn532_uart` component) are supported at the moment due to required modifications that haven't been ported to other protocols or chips

> [!NOTE]
🧰 Tools
🪛 LanguageTool

[style] ~242-~242: For conciseness, consider replacing this expression with an adverb.
Context: ...U(pn532_uart component) are supported at the moment due to required modifications that have...

(AT_THE_MOMENT)

🪛 markdownlint-cli2 (0.21.0)

[warning] 243-243: Blank line inside blockquote

(MD028, no-blanks-blockquote)

Comment thread README.md
rx_pin: 16
baud_rate: 115200

pn532_spi:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Wrong component name in UART example — use pn532_uart instead of pn532_spi.

The UART configuration example uses pn532_spi: as the component key, which would configure the SPI transport, not UART. Users following this example would get a misconfigured setup.

📝 Proposed fix
-pn532_spi:
+pn532_uart:
   id: nfc_uart_module
   uart_id: uart_nfc
   update_interval: 100ms
   keep_rf_on: true
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pn532_spi:
pn532_uart:
id: nfc_uart_module
uart_id: uart_nfc
update_interval: 100ms
keep_rf_on: true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants