hd44780: fix state corruption during 4-bit read-back (busy-flag polling)#583
hd44780: fix state corruption during 4-bit read-back (busy-flag polling)#583felixmertins wants to merge 3 commits into
Conversation
hd44780_process_read() raises IRQ_HD44780_ALL with the data nibble the part is driving back to the AVR. That IRQ is also registered on our own hd44780_pin_changed_hook, whose ALL case decodes the value as a complete pin set (D4-D7, RS, E, RW). A status/data read therefore clobbered b->pinstate: RW fell back to 0, so the firmware's next E strobe - e.g. the discard strobe of a 4-bit busy-flag poll - was processed as a data write of whatever floated on the bus, corrupting the nibble phase and the display RAM. Guard the ALL case with the existing REENTRANT flag, exactly like the D0...D7 case already does: while we are the ones driving the bus, our own raise must not be decoded as input. External users of IRQ_HD44780_ALL are unaffected. Found by running firmware that polls the busy flag in 4-bit mode under simavr; every read corrupted the following write.
The D0...D7 case skipped the pinstate update while REENTRANT is set, i.e. while hd44780_process_read() drives the data lines back to the AVR. The raised values still propagate to the connected (bidirectional) ioport pin IRQs, which are FILTERED and cache them. When the firmware subsequently writes a nibble containing the same logic level on a pin, avr_ioport raises nothing (no change from the cached value), the part never hears about it, and the write is latched with stale data bits. Observed as single corrupted bits in characters written to DDRAM line 2 right after busy polls (the read-back address bits matched the written data bits often enough to drop them). Updating pinstate with the value we drive ourselves is safe: it is the actual bus level, nothing in the read path samples the data pins, and the raise cannot loop (our D IRQs are FILTERED, so the bounce through the ioport terminates after one hop).
|
It looks like LLM outout. What shows it is correct? |
The firmware brings the LCD up in 4-bit mode and polls the busy flag before every byte, wired bidirectionally like examples/board_hd44780/charlcd.c. The test then compares DDRAM against the strings the firmware wrote. On master without the two fixes the init sequence derails and DDRAM stays empty. With only the first fix applied, single bits are still dropped from characters written after busy polls: DDRAM line 2 reads "SO......ET" instead of "SOFT RESET", the corruption pattern from the issue. With both fixes the test passes. The test links examples/parts/hd44780.c, which is not part of libsimavr, hence the dedicated make rule.
|
Yes, I use an LLM when writing things up. The findings themselves come from debugging my own firmware against this part, but fair point, prose doesn't prove anything. So I've added a regression test to the branch instead. The firmware (tests/atmega644_hd44780.c) brings the LCD up in 4-bit mode and polls the busy flag before every byte, wired bidirectionally like examples/board_hd44780/charlcd.c. The test driver then compares DDRAM against what was written. Results:
The mechanism is also easy to check by hand on master: hd44780_process_read() raises IRQ_HD44780_ALL with the read-back nibble (hd44780.c line 313), and the ALL case of the part's own pin hook (line 370) decodes RS, RW and E out of that value, without the REENTRANT guard that the D0..D7 case right below it has. So every status read rewrites the part's idea of RW. The test hooks into the existing tests/ machinery and runs as part of make run_tests. It needs one extra make rule because it links examples/parts/hd44780.c, which is not part of libsimavr. |
Fixes #582
Two independent defects made the HD44780 part unusable with firmware
that polls the busy flag in 4-bit mode (standard bidirectional wiring as
in
examples/board_hd44780). Details, cycle traces and analysis in thelinked issue; summary:
hd44780: don't reinterpret our own readback as pin input—hd44780_process_read()raisesIRQ_HD44780_ALLwith the nibble itdrives back to the AVR; the part's own pin hook decoded that value as
a full pin set and clobbered RS/RW/E in
pinstate, so the discardstrobe of a busy poll was processed as a garbage write. The
ALLcase now honours
HD44780_PRIV_FLAG_REENTRANT, exactly like theD0 ... D7case already does.hd44780: track the data pins while driving a read back— theD0 ... D7early-return underREENTRANTleftpinstatestalewhile the FILTERED ioport pin IRQs cached the read-back level; a
subsequent firmware write of the same logic level produced no edge
notification and was latched with old data bits (single-bit
corruption in characters written after busy polls).
pinstatenowtracks the value the part drives itself — it is the real bus level,
nothing in the read path samples it, and the FILTERED D IRQs keep
the bidirectional connection loop-free.
Validation: my AVR project uses an HD44780 driver that busy-polls before
every byte. Unpatched, the display stays empty / fills with garbage
(traces in the issue). With these two commits, the full LCD regression
suite (init sequence, both DDRAM lines, writes right after busy polls)
passes with no board-side workarounds. No change for firmwares that use
timed writes only, and external users of
IRQ_HD44780_ALLas an inputare unaffected.