Potential emu_timer::enable issues/cleanup

[MooglyGuy]

The behavior of emu_timer::enable is to unschedule the timer if called with false, and to re-schedule the timer if called with true, albeit with the originally-set expiration time. This means, for example, if the following sequence occurs:

• adjust(attotime::from_seconds(1));
• enable(false);
• 5 seconds elapse
• enable(true);
  The timer will fire instantly, rather than 1 second after the timer is re-enabled.

The following drivers and devices all make calls to emu_timer::enable with a value of either true or 1, and are grouped based on observed behavior:

Unnecessary calls to enable(true) due to an immediately preceding (or following) call to adjust or reset, or due to the timer never being disabled in the first place:

• src/devices/bus/ata/atahle.cpp
• src/devices/bus/ss50/mp4.cpp
• src/devices/bus/ti99/joyport/handset.cpp
• src/devices/bus/ti99/joyport/mecmouse.cpp
• src/devices/machine/corvushd.cpp
• src/mame/bitcorp/gamate.cpp
• src/mame/ensoniq/esqpanel.cpp
• src/mame/skeleton/gameking.cpp
• src/mame/svision/svision.cpp
• src/mame/ti/ti85_m.cpp
• src/mame/usp/patinho_feio.cpp
• src/mame/shared/xbox_nv2a.cpp
• src/mame/shared/xbox_pci.cpp
• src/mame/shared/xbox_usb.cpp

Devices or drivers which have a periodic timer that can be temporarily suspended, which may result in the timer spuriously firing on re-enable, regardless of periodicity, if a full timer period has elapsed since it was disabled:

• src/devices/bus/vip/vp550.cpp
• src/devices/imagedev/microdrv.cpp
• src/devices/machine/icm7170.cpp
• src/devices/machine/mccs1850.cpp
• src/devices/machine/rtc4543.cpp
• src/devices/machine/upd4992.cpp
• src/mame/act/victor9k_fdc.cpp
• src/mame/pdp1/pdp1.cpp

Devices or drivers which appear to erroneously assume that a timer, when re-enabled, will have the same remaining duration as when it was disabled:

• src/devices/machine/6840ptm.cpp
• src/devices/machine/mc68901.cpp
• src/devices/machine/upd1990a.cpp
• src/mame/aviion/aviion88k.cpp
• src/mame/konami/gticlub.cpp
• src/mame/konami/hornet.cpp
• src/mame/konami/nwk-tr.cpp
• src/mame/nintendo/pokemini.cpp

src/devices/machine/am9513.cpp is a special case as the current behavior is correct, but the code itself could be changed for clarity. The case where m_freq_timer_selected[f] is non-zero is meaningless as the timer will have been adjusted (and therefore enabled) immediately prior, and there is no point in adjusting the timer if m_freq_timer_selected[f] is zero as it is never enabled outside of the relevant function.

I'll remove the unnecessary enable calls from the first group myself, and will be trying to fix up the latter two groups of devices and drivers as well. However, I'd feel more comfortable if the relevant owners of the latter two groups would take point, so tagging in @curt-coder, @angelosa, @happppp, @rb6502, @wilbertpol, and @villedevs if any of you want to take a stab at it yourselves. For the drivers and devices in the final group (incorrect assumption of timer suspension), the solution I've implemented for 6840ptm.cpp in my local tree is:

• When disabling a timer, take note of remaining(), stash it in a member variable, then call adjust(attotime::never) on the timer.
• When calling adjust on the timer with some non-never value, clear the stashed value to attotime::never
• When re-enabling a timer, use the stashed value (if the timer is not already running and enabled).

[MooglyGuy]

I can't seem to assign this ticket to myself as I'm not part of the organization, so if someone would be so kind as to assign it to me, I'd appreciate it.

[ajrhacker]

Note that changing enable(false) to adjust(attotime::never) will break things that rely on enabled(), since that predicate actually returns true in the latter case.

[MooglyGuy]

I'm aware of that, and the number of drivers and devices that actually do that is minimal.

[cuavas]

This is really a case of confusing design – there are two states where a timer won’t fire:

• Disabled - m_enabled is false
• Unscheduled – m_expire.is_never() returns true

Persistent timers are disabled and unscheduled on creation. When a non-periodic timer fires, it enters the disabled state. As a quirky implementation detail, all timers (periodic or not) have their start time set to the former expiry time when they fire – I wouldn’t be surprised if at least one system depends on this, expecting elapsed to report the time since an expired non-periodic timer fired, as opposed to the time since it was last adjusted.

Calling reset or adjust always enables the timer, even when unscheduling it. Calling reset or adjust always sets the timer’s start time, affecting the result of future calls to elapsed.

The reset member function itself is confusing, as it defaults to unscheduling the timer with the default argument, but can also be used to change the current expiry without affecting the parameter value or periodicity. The adjust member function is also confusing as it always sets the current expiry, parameter value, and periodicity. If anything, it would make more sense if the names were reversed: reset adjusts the timer, while adjust completely resets it.

This distinction between disabled and unscheduled timers can lead to timers being used for two purposes: the primary role of a timed event trigger, and also for calculating time until/since some time point independent of whether the timer is being used to trigger an action.

Timers also suffer from lack of documentation. Combined with behaving somewhat unintuitively, this has probably led to the evident developer confusion when it comes to appropriate use of enable.

It may actually be possible to eliminate the disabled state altogether to reduce confusion. Non-periodic timers could be unscheduled after firing rather than being disabled. Even just removing the ability to enable/disable a timer directly may reduce developer confusion.