Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: Duplicated last activated item after manual refresh #264

Closed
feketeimre opened this issue Nov 14, 2024 · 0 comments · Fixed by #286
Closed

[BUG]: Duplicated last activated item after manual refresh #264

feketeimre opened this issue Nov 14, 2024 · 0 comments · Fixed by #286
Labels
bug Something isn't working

Comments

@feketeimre
Copy link

What happened?

Im not sure if im doing something wrong or not but heres what i have experienced with the following code. I dont feel that it is hardware specific, maybe someone can confirm or deny.
I have 4 toggle items: Start all, Start 0, Start 1, Start 2. i can toggle things individually by Start 1,2,3 or toggle them together by Start all. With Start all i use setIsOn on the display items Start 0 1 2 then call refresh to show the changed items. After refresh the last activated menu item is duplicated on the last line of the lcd, in this case Start all. What should happen is the last line should correctly show Start 2:ON
After i move the cursor the display goes back to its correct state.
Heres what i see on the lcd before and after activation of the first item:
image
And heres the code:

#include <LiquidCrystal.h>
#include <SimpleRotary.h>
#include <ItemToggle.h>
#include <LcdMenu.h>
#include <MenuScreen.h>
#include <input/SimpleRotaryAdapter.h>
#include <display/LiquidCrystalAdapter.h>
#include <renderer/CharacterDisplayRenderer.h>

#define LCD_COLS 20 
#define LCD_ROWS 4
#define LCD_PINS_D7 59
#define LCD_PINS_D6 63
#define LCD_PINS_D5 64
#define LCD_PINS_D4 40
#define LCD_PINS_RS 44
#define LCD_PINS_EN 42
#define BTN_EN1 51
#define BTN_EN2 49
#define BTN_ENC 66

LiquidCrystal lcd(LCD_PINS_RS, LCD_PINS_EN, LCD_PINS_D4, LCD_PINS_D5, LCD_PINS_D6, LCD_PINS_D7);
SimpleRotary encoder(BTN_EN1, BTN_EN2, BTN_ENC);
LiquidCrystalAdapter lcdAdapter(&lcd, LCD_COLS, LCD_ROWS);
CharacterDisplayRenderer renderer(&lcdAdapter, LCD_COLS, LCD_ROWS);
LcdMenu menu(renderer);
SimpleRotaryAdapter rotaryInput(&menu, &encoder);

void startall(bool isOn); //this will set startItem0-2 ON or OFF
void start0(bool isOn); 
void start1(bool isOn); 
void start2(bool isOn); 

ItemToggle startAll = ItemToggle("Start all", startall);
ItemToggle startItem0 = ItemToggle("Start 0", start0);
ItemToggle startItem1 = ItemToggle("Start 1", start1);
ItemToggle startItem2 = ItemToggle("Start 2", start2);

// clang-format off
MENU_SCREEN(mainScreen,mainItems,
    &startAll,
    &startItem0,
    &startItem1,
    &startItem2
    );
// clang-format on

void setup() {
  renderer.begin();
  menu.setScreen(mainScreen);
}

void loop() {
  rotaryInput.observe();
  }

void startall(bool isOn){  
    startItem0.setIsOn(isOn);  // turn on item 0
    //do thing 0
    startItem1.setIsOn(isOn); // turn on item 1
    //do thing 1
    startItem2.setIsOn(isOn); // turn on item 2
    //do thing 2
    menu.refresh();  //refresh the display to show changes, after this the last activated item is duplicated
  };

// you can reproduce the bug with the below function too, it seems you only need to call refresh to reproduce the bug and not change any item
/*
void startall(bool isOn){  
    menu.refresh(); 
};*/

  void start0(bool isOn){ //do thing 0} 
  void start1(bool isOn){ //do thing 1}
  void start2(bool isOn){ //do thing 2}

Version

5.3.1

What environment are you seeing the problem in?

Arduino

Board type

Mega2560

Relevant log output

No response

@feketeimre feketeimre added the bug Something isn't working label Nov 14, 2024
mriveralee added a commit to mriveralee/LcdMenu that referenced this issue Jan 4, 2025
mriveralee added a commit to mriveralee/LcdMenu that referenced this issue Jan 4, 2025
@forntoh forntoh closed this as completed in 1835741 Jan 4, 2025
forntoh added a commit that referenced this issue Jan 4, 2025
## Description
This PR fixes #264. I reproduced this bug and have found the issue:
`ItemToggle::toggle()` executes its `callback` function before it
redraws. When `menu.refresh()` is in the callback function, it changes
the cursor row position to the max row. Then ItemToggle calls its draw
to duplicate the item on the last visible row.

The simple solution here is to move the callback after the redraw. A
similar approach is used in ItemInput,
`ItemInput::back()`(https://github.com/forntoh/LcdMenu/blob/master/src/ItemInput.h#L189)
, which calls `draw(...)` _first_ and then execute the callback.

## How Has This Been Tested?

I used the provided code in #264 with an Arduino Nano, LCD Display 4x20
I2C , and Rotary Encoder. I tested by running the code, toggle the first
item to trigger the `menu.refresh()` call, and visually verified the
problem is resolved. I also tested the individual toggle examples to
make sure the behavior works as expected.


## Additional Notes

---

### Checklist

<!-- Note: Without all of these items checked the PR will not be
reviewed. -->

#### General Requirements

- [X] I have kept this PR in draft until all the required tasks are
completed.
- [X] I have reviewed the [contributing guidelines](/CONTRIBUTING.md)
for this project.
- [X] I have tagged this PR with `breaking-change` if it introduces a
breaking change.
- [X] I have checked that this PR does not introduce any breaking
changes unless explicitly stated.
- [X] I have checked that changes generate no new warnings.
- [X] I have performed a self-review of my own code
- [X] I have built and tested **ALL** the examples to ensure that I
haven't broken anything.

#### Bug Fix

<!-- Delete this section if it doesn't apply to your PR. -->
- [X] **This PR is a bug fix.**
- [X] I have tagged this PR with `bugfix`.
- [X] I have added a link to the bug in the description.
- [X] I have added tests that prove my fix is effective.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Summary by CodeRabbit

- **Bug Fixes**
- Improved toggle state management to ensure logging and drawing
operations accurately reflect the updated item state after toggling.
  
- **Chores**
- Updated GitHub Actions workflow for lint checking to disable automatic
fixes and commits during the linting process.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant