Feature/d1l rx deadlock fix #50

2025-12-28T16:24:25+02:00

okapteinis commented

2025-12-28 16:24:25 +02:00

(Migrated from github.com)

Description

Type of Change

Bug fix (non-breaking change fixing an issue)
New feature (non-breaking change adding functionality)
Breaking change (fix or feature causing existing functionality to change)
Documentation update
Hardware support addition
Performance improvement

Testing Performed

Builds successfully on affected platforms
Tested on physical hardware
Verified no new compiler warnings
Checked memory usage
Example applications still work
Documentation updated

Hardware Tested

Device:
Firmware variant:
Test results:

Code Quality Checklist

Follows existing code style
No dynamic memory allocation in inappropriate places
No reformatting of existing code
Added comments for non-obvious logic
Updated documentation
CHANGELOG.md updated if applicable

Breaking Changes

Screenshots/Logs

Additional Notes

## Description  ## Type of Change  - [ ] Bug fix (non-breaking change fixing an issue) - [ ] New feature (non-breaking change adding functionality) - [ ] Breaking change (fix or feature causing existing functionality to change) - [ ] Documentation update - [ ] Hardware support addition - [ ] Performance improvement ## Related Issues  ## Testing Performed  - [ ] Builds successfully on affected platforms - [ ] Tested on physical hardware - [ ] Verified no new compiler warnings - [ ] Checked memory usage - [ ] Example applications still work - [ ] Documentation updated ## Hardware Tested  - Device: - Firmware variant: - Test results: ## Code Quality Checklist - [ ] Follows existing code style - [ ] No dynamic memory allocation in inappropriate places - [ ] No reformatting of existing code - [ ] Added comments for non-obvious logic - [ ] Updated documentation - [ ] CHANGELOG.md updated if applicable ## Breaking Changes  ## Screenshots/Logs  ## Additional Notes

gemini-code-assist[bot] commented

2025-12-28 16:24:45 +02:00

(Migrated from github.com)

Summary of Changes

Hello @okapteinis, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request delivers a crucial fix for the SenseCAP Indicator D1L, resolving a severe I2C mutex deadlock that rendered the device's receiver non-functional. The core change involves upgrading the I2C mutex to a recursive type, which correctly handles nested locking scenarios inherent to the D1L's hardware architecture. Beyond the functional repair, the PR significantly improves the user experience by adding detailed documentation for D1L setup and troubleshooting, alongside a new, robust serial monitoring utility.

Highlights

I2C Mutex Deadlock Resolution: A critical deadlock on the SenseCAP Indicator D1L's I2C bus, which caused complete RX (receive) failure, has been identified and fixed. The issue stemmed from nested locking attempts on a regular mutex.
Recursive Mutex Implementation: The solution involved converting the d1l_i2c_mutex to a recursive mutex (xSemaphoreCreateRecursiveMutex()) and introducing a new RecursiveSemaphoreLockGuard class to handle its acquisition and release safely.
Restored RX Functionality: The implemented fix successfully restored full receive functionality on the D1L hardware, enabling neighbor discovery and proper ping operations, resolving the 'recv:0' issue.
Enhanced Documentation: A comprehensive D1L_GUIDE.md has been added, providing detailed flashing instructions, troubleshooting steps, and a thorough post-mortem analysis of the I2C deadlock for future reference.
New Monitoring Tool: A Python-based serial logger (d1l_logger.py) has been introduced to provide more reliable and event-aware serial monitoring for the D1L, especially on macOS.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩︎

## Summary of Changes Hello @okapteinis, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a crucial fix for the SenseCAP Indicator D1L, resolving a severe I2C mutex deadlock that rendered the device's receiver non-functional. The core change involves upgrading the I2C mutex to a recursive type, which correctly handles nested locking scenarios inherent to the D1L's hardware architecture. Beyond the functional repair, the PR significantly improves the user experience by adding detailed documentation for D1L setup and troubleshooting, alongside a new, robust serial monitoring utility. ### Highlights * **I2C Mutex Deadlock Resolution**: A critical deadlock on the SenseCAP Indicator D1L's I2C bus, which caused complete RX (receive) failure, has been identified and fixed. The issue stemmed from nested locking attempts on a regular mutex. * **Recursive Mutex Implementation**: The solution involved converting the `d1l_i2c_mutex` to a recursive mutex (`xSemaphoreCreateRecursiveMutex()`) and introducing a new `RecursiveSemaphoreLockGuard` class to handle its acquisition and release safely. * **Restored RX Functionality**: The implemented fix successfully restored full receive functionality on the D1L hardware, enabling neighbor discovery and proper ping operations, resolving the 'recv:0' issue. * **Enhanced Documentation**: A comprehensive `D1L_GUIDE.md` has been added, providing detailed flashing instructions, troubleshooting steps, and a thorough post-mortem analysis of the I2C deadlock for future reference. * **New Monitoring Tool**: A Python-based serial logger (`d1l_logger.py`) has been introduced to provide more reliable and event-aware serial monitoring for the D1L, especially on macOS. <details> <summary>Using Gemini Code Assist</summary> The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either `/gemini <command>` or `@gemini-code-assist <command>`. Below is a summary of the supported commands on the current page. Feature | Command | Description --- | --- | --- Code Review | `/gemini review` | Performs a code review for the current pull request in its current state. Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state. Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. Help | `/gemini help` | Displays a list of available commands. Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a `.gemini/` folder in the base of the repository. Detailed instructions can be found [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up [here](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG). You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the [Gemini Code Assist IDE Extension](https://cloud.google.com/products/gemini/code-assist). </details> [^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487).

gemini-code-assist[bot] (Migrated from github.com) reviewed

2025-12-28 16:26:31 +02:00

gemini-code-assist[bot] (Migrated from github.com) left a comment

Code Review

This pull request provides a critical fix for a deadlock issue on the D1L variant by correctly switching to a recursive mutex for I2C bus protection. The implementation is solid, introducing a new RAII wrapper for recursive mutexes and applying it consistently. The addition of timeouts for mutex acquisition is a commendable improvement for system robustness. The detailed documentation, including a post-mortem, is excellent. I've identified a couple of issues: the new logger script contains hardcoded paths, which impacts portability, and a code snippet in the new documentation is slightly out of sync with the implementation. Overall, this is a high-quality contribution that resolves a critical bug.

## Code Review This pull request provides a critical fix for a deadlock issue on the D1L variant by correctly switching to a recursive mutex for I2C bus protection. The implementation is solid, introducing a new RAII wrapper for recursive mutexes and applying it consistently. The addition of timeouts for mutex acquisition is a commendable improvement for system robustness. The detailed documentation, including a post-mortem, is excellent. I've identified a couple of issues: the new logger script contains hardcoded paths, which impacts portability, and a code snippet in the new documentation is slightly out of sync with the implementation. Overall, this is a high-quality contribution that resolves a critical bug.

docs/hardware/D1L_GUIDE.md

					
				@ -0,0 +264,4 @@

				                locked = true;

				            } else {

				                Serial.println("[CRITICAL] RecursiveSemaphoreLockGuard: Failed to acquire mutex!");

				            }

The error logging in this documentation snippet is out of sync with the actual implementation in freertos_util.h. The implementation logs a more detailed message including the timeout duration and a second message about a potential deadlock, all within an #ifdef ARDUINO block. Keeping the documentation aligned with the code is important for accuracy.

            } else {
                #ifdef ARDUINO
                Serial.println("[CRITICAL] RecursiveSemaphoreLockGuard: Failed to acquire mutex within 5s timeout!");
                Serial.println("[CRITICAL] Possible deadlock or priority inversion detected!");
                #endif
            }

![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg) The error logging in this documentation snippet is out of sync with the actual implementation in `freertos_util.h`. The implementation logs a more detailed message including the timeout duration and a second message about a potential deadlock, all within an `#ifdef ARDUINO` block. Keeping the documentation aligned with the code is important for accuracy. ```suggestion } else { #ifdef ARDUINO Serial.println("[CRITICAL] RecursiveSemaphoreLockGuard: Failed to acquire mutex within 5s timeout!"); Serial.println("[CRITICAL] Possible deadlock or priority inversion detected!"); #endif } ```

tools/d1l_logger.py

					
				@ -0,0 +9,4 @@

				import time

				from datetime import datetime

				SERIAL_PORT = "/dev/cu.usbserial-1110"

The SERIAL_PORT is hardcoded. This harms the script's portability, as the serial port name can vary significantly between different machines, operating systems, and even connections. It's best practice to make this configurable, for instance by passing it as a command-line argument using Python's argparse module.

![high](https://www.gstatic.com/codereviewagent/high-priority.svg) The `SERIAL_PORT` is hardcoded. This harms the script's portability, as the serial port name can vary significantly between different machines, operating systems, and even connections. It's best practice to make this configurable, for instance by passing it as a command-line argument using Python's `argparse` module.

tools/d1l_logger.py

					
				@ -0,0 +11,4 @@

				SERIAL_PORT = "/dev/cu.usbserial-1110"

				BAUD_RATE = 115200

				LOG_DIR = "/Users/ojarskapteinis/Documents/Kods/MeshCore/monitoring_logs"

The LOG_DIR is hardcoded to an absolute path on a specific user's machine. This is a critical issue as it will cause the script to fail for any other user on any other machine. This should be changed to a relative path or made configurable via command-line arguments.

LOG_DIR = "monitoring_logs"

![critical](https://www.gstatic.com/codereviewagent/critical.svg) The `LOG_DIR` is hardcoded to an absolute path on a specific user's machine. This is a critical issue as it will cause the script to fail for any other user on any other machine. This should be changed to a relative path or made configurable via command-line arguments. ```suggestion LOG_DIR = "monitoring_logs" ```

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

ojars/MeshCore!50

Rows
Columns