Bugfix: 4 bars appear when no indicators are being shown

[forntoh]

fixes #247

• New Features
  • Enhanced workflow for publishing releases, improving versioning and release tagging.
• Bug Fixes
  • Adjusted character rendering logic for better visibility of up and down indicators.
• Documentation
  • Updated documentation for CharacterDisplayRenderer to clarify reserved slots for arrow icons and available custom character slots.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 5155935 and 7a5e2a1.

📒 Files selected for processing (3)

• .github/workflows/publish.yml (3 hunks)
• src/renderer/CharacterDisplayRenderer.cpp (2 hunks)
• src/renderer/CharacterDisplayRenderer.h (1 hunks)

🔇 Additional comments (7)

src/renderer/CharacterDisplayRenderer.cpp (2)

59-59: Corresponding indicator values updated to match new character slots.

The indicator values have been correctly aligned with the new character slot assignments (0 for up arrow, 1 for down arrow, space for no arrow).

---

20-21: Fix for LCD display artifacts by optimizing character slot usage.

The change to use slots 0 and 1 instead of 1 and 2 for arrow characters is the correct fix for the reported issue. This prevents potential initialization artifacts that were causing the four bars to appear on some LCD displays.

Let's verify there are no other character slot assignments that might conflict:

✅ Verification successful

Character slot changes are safe to implement

The verification confirms that slots 0 and 1 are only used for the up and down arrows in CharacterDisplayRenderer.cpp, with no conflicting usage across the codebase. The display adapters (SSD1803A_I2CAdapter, LiquidCrystalAdapter, and LiquidCrystal_I2CAdapter) only provide the implementation of the createChar interface without any hardcoded character assignments.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any other createChar calls that might conflict with slots 0 and 1
rg "createChar\s*\(\s*[01]\s*," --type cpp

Length of output: 286

---

Script:

#!/bin/bash
# Let's check for any other character-related operations in the codebase
rg -A 3 "createChar|write\s*\(\s*[0-7]\s*\)" --type cpp

Length of output: 1657

---

Script:

#!/bin/bash
# Let's check how these characters are used in the display logic
rg -A 5 "write\s*\(\s*[^,]*\)" src/renderer/CharacterDisplayRenderer.cpp

Length of output: 74

src/renderer/CharacterDisplayRenderer.h (1)

59-60: Documentation update correctly reflects character slot assignments.

The change to use slots 0 and 1 for arrow icons is a good fix that aligns with common LCD character display implementations. This should resolve the reported issue of unwanted bars appearing on the display.

To ensure the fix works across platforms, please verify:

Please test this change on both Arduino Uno and ESP32 platforms to confirm:

1. The unwanted bars no longer appear
2. Arrow indicators display correctly
3. Custom characters in slots 2-7 work as expected

✅ Verification successful

Based on the shell script results, I can now generate the final response:

Character slot assignments are correctly implemented across the codebase

The verification confirms:

• CharacterDisplayRenderer.cpp correctly uses slots 0 and 1 for up/down arrows
• No conflicting character slot assignments found in adapter implementations
• Custom character creation is consistently handled through the CharacterDisplayInterface

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any other character slot assignments that might need updating
rg -A 2 "createChar\(" 

# Look for any hardcoded character codes that might conflict
rg --type cpp "(0x[0-7]|[0-7])\s*,\s*[\w>]+"

Length of output: 3039

.github/workflows/publish.yml (4)

67-67: LGTM! Good improvement in branch reference handling.

Using ${{ github.ref }} instead of a hardcoded branch makes the workflow more flexible and maintainable.

---

132-133: LGTM! Good security practice in release targeting.

Using target_commitish: ${{ github.sha }} is a good practice as it ensures the release is created at the exact commit, preventing potential race conditions.

---

Line range hint 1-158: Verify the relationship between workflow changes and bug fix.

While the workflow improvements are valuable, they appear unrelated to the stated PR objective of fixing the LCD display bug with ESP32 support. Consider:

1. Splitting these changes into a separate PR to maintain clear change history
2. Adding documentation explaining why these workflow changes are necessary for the bug fix

#!/bin/bash
# Check if there are related changes in LCD display files
rg -l "LCD|Display|ESP32" --type cpp --type h

---

24-25: Verify if explicit ref is necessary in checkout action.

The ref: ${{ github.ref }} configuration might be redundant since actions/checkout by default checks out the ref/SHA that triggered the workflow.

✅ Verification successful

The explicit ref in checkout action is necessary - keep it

The explicit ref: ${{ github.ref }} configuration is actually necessary in this workflow because:

1. This is a manually triggered workflow (workflow_dispatch) where version updates and git operations are performed
2. The workflow needs to ensure it's working on the exact reference that triggered the workflow, especially for the subsequent git operations (commit, tag, push)
3. The workflow performs critical operations like publishing to PlatformIO registry and creating GitHub releases

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are specific cases in the codebase where the ref needs to be explicit
rg -l "refs/heads|refs/tags" .github/workflows/

Length of output: 702

---

Script:

#!/bin/bash
# Let's check the workflow file content to understand the context better
cat .github/workflows/publish.yml

Length of output: 5477