HD44780: Implement timing (busy/status and blink interval) based on the input clock. Add notes about device variants. [Lord Nightmare] #11776
Conversation
Lord-Nightmare
force-pushed
the
hd44780_timing
branch
from
b329ae6
to
842a7d9
Compare
…he input clock. Add notes about device variants. [Lord Nightmare]
Lord-Nightmare
force-pushed
the
hd44780_timing
branch
from
842a7d9
to
d376c9c
Compare
There was a problem hiding this comment.
- Isn’t the whole point of
m_busy_factorto allow adjustment for different clock frequencies without using the configured clock frequency? Shouldn’t it be unnecessary if the configured clock frequency is used to calculate how long the device will be busy for? - Should you be quantising the busy time to intervals of the clock frequency?
- Why aren’t you using
attotime::from_ticks?
|
Please don’t force push unnecessarily when changing code for pull requests. It just makes it unnecessarily hard for reviewers because it’s harder to see what changed each time. |
This force-push was necessary because a change upstream made it un-merge-able. Absolutely no functional changes (or really any changes at all, other than fixing the merge conflict because git got confused) were made. |
|
And that's doable by fixing thru |
src/devices/video/hd44780.cpp
Outdated
|
|
||
| usec = float(usec) * m_busy_factor + 0.5; | ||
| m_busy_timer->adjust(attotime::from_usec(usec)); | ||
| m_busy_timer->adjust(attotime::from_hz(double(clock() / cycles) * m_busy_factor)); |
There was a problem hiding this comment.
This will invert the result with busy_factor?
eg. busy factor 0.5, will make the busy flag duration twice longer, or 0.0 would mean 0Hz which is attotime::never.
There was a problem hiding this comment.
This should be fixed now, although I'm going to see if I can completely remove the whole busy factor system in favor of just adjusting the clock for the few drivers that use it, in the future
There was a problem hiding this comment.
Does anything have a busy factor of 0.0? If yes, then the fix is a divide by 0 crash.
There was a problem hiding this comment.
Nothing does, no.
And if I can make busy_factor go away completely, this will be a non-issue.
* use attotime::from_ticks * properly capitalize SI suffixes * fixed busy factor "polarity" in busy timer adjust, pending eventually removing busy factor entirely in the future
I'm going to see if I can safely completely remove the busy factor from the few drivers that use it and ditch it entirely. I've switched to attotime::from_ticks. |
|
Why didn’t you wait for @Lord-Nightmare to try getting rid of the fudge factor? Also, please look at the commit message of the commit. Can people please think about release notes? |
|
I told LN on discord this good for a 1st step. |
Look at the squashed commit – the message appears twice. |
|
Ah I see now, yes the PR title is the 1st line of the commit msg. |
When you go to merge the PR there are two edit fields. The first (one-line) edit field is the first line of the commit message. A blank line is automatically added after this. The second (multi-line) edit field is the rest of the commit message. |
…he input clock. Add notes about device variants. [Lord Nightmare] (mamedev#11776) * HD44780: Implement timing (busy/status and blink interval) based on the input clock. Add notes about device variants. [Lord Nightmare]
HD44780: Implement timing (busy/status and blink interval) based on the input clock. Add notes about device variants. [Lord Nightmare]