Re: [hatari-devel] Recent Hatari change problems

[Thomas Huth <th.huth@xxxxxxxxx>]

On 11.04.2018 22:32, Eero Tamminen wrote:
> Hi,
> 
> On 04/11/2018 11:08 AM, Thomas Huth wrote:
>> On 11.04.2018 01:23, Eero Tamminen wrote:
>>> You have been doing some changes to the command line options,
>>> e.g. changing --xbios-intercept from toggle to just unconditionally
>>> enabling it.
>>>
>>> Note that all command line options are also options for the debugger.
>>> Things that you toggle on from command line, can be toggled off from
>>> debugger.  The earlier functionality was:
>>> ----------------------
>>> $ ./src/hatari --bios-intercept
>>> XBios 11/20/255 Hatari versions enabled: Dbmsg(), Scrdmp(),
>>> HatariControl().
>>> ...
>>> You have entered debug mode. Type c to continue emulation, h for help.
>>> ...
>>>> setopt --bios-intercept
>>> XBios 11/20/255 Hatari versions disabled.
>>
>> Ah, ok, now it makes more sense that this option was written in such a
>> cumbersome way ;-)
> 
> While I appreciate you adding more automated testing to Hatari...
> 
> Before making random changes to Hatari's public interface,
> *please* discuss them on the mailing list.

Yeah, sorry, I already wanted to write a mail about that new "make test"
regression test suite last weekend already, but did not have enough
spare time to do so. I'll try to write a summary during the next weekend...

> E.g. changing option output to happen only with the lowest "debug"
> level is just broken.  It means that user doesn't get any feedback
> when he changes something in debugger.  Please fix that.

Honestly, I consider these messages as *debug* messages. So I'm not
convinced that we should spam the output with these messages. At least
not while parsing the command line option. Got to ponder a little bit on
whether it makes sense to always print these messages during the
debugger session or not.

> In some cases it's pretty important information:
>> setopt --disasm 0xf
> Changed CPU disassembly output flags from 15 to 0.
>> setopt --disasm 15
> Changed CPU disassembly output flags from 0 to 15.
> 
> With your change, user wouldn't know that given option doesn't accept
> hex values for the bitmask, but interprets them as decimals. I.e.
> options output should be at least the same as Hatari default
> log level.

Well, in that case, there should be an *error* message instead if there
there are unparsable characters in the input.

>> Anyway, "toggling" is a bad idea here, since the users first have to
>> change the option to know in which state it was.
> 
> It's off by default and doesn't have config option, because having
> it enabled by default would be a security issue.

Sure, but still, the user has to remember the current state of the
option if it is just toggling the state. It's way more useful if you can
directly say whether you want to switch it on or off.

>> I've now changed it to a proper "boolean" option
>> ("--bios-intercept on|off") which should be
>> more straight forward and fix this problem.
> 
> That breaks any programs that were using the above XBios() call.

Honestly, do you know any such program?

> If such changes are deemed necessary, they must be listed in
> the release notes (maybe under "API changes" heading).
> When updating command line options, please update both the Hatari
> HTML manual and manual page, not just one of them.

Yeah, right, I guess I shouldn't rush patches during short breaks in my
busy week. I'll fix it at the weekend.

> Btw. Why the commit to make XBios test "more reliable", changes Hatari
> to be run with "--cpuclock 32" instead of using "--fast-forward on"?

The basic idea is to "test" more command line parameters.
"--fast-forward" is already tested by the bus error tester, so I wanted
to check "--cpuclock" in one of the testers, too. The test runs
reasonable fast anyway so that you won't notice a big difference here.

> FYI: Some other options that can be used to make emulation faster
> in addition to "--fast-forward yes" and "--sound off":
> * "--borders off" when overscan effects don't need to be visible
> * "--spec512 0" when Spec512 effects don't need to be visible
> * "--zoom 1" to lessen Hatari screen conversion overhead
> * "--disable-video yes" when display output isn't needed

The tests are run with SDL_VIDEODRIVER=dummy anyway, so I'd not expect
much speed improvements for these options. But if you see an improvement
with one of these options, feel free to add them to the run_tests.sh script.

> * "--dsp off" when tests don't use DSP (for Falcon)

No test is using --machine falcon yet.

> * "--fastfdc yes" when tests don't use floppy

No test is using the FDC yet, so there should not be any difference here.

> * "--fast-boot yes" when TOS memory detection isn't needed
> * "--timer-d yes" when tests don't depend on timer-D interval

Tests run without TOS, so I'd also not expect any difference with these.

> I would also recommend "--mousewarp off" to avoid Videl code
> "randomly" moving mouse on screen when emulation is booted.

Again: Test run without TOS, so no mouse involved here. I'll summarize
this in my mail about "make test" on the next weekend.

> As to converting dbmsg.c tester to assembly... While one reason
> for the tests is making sure that Hatari functionality doesn't
> regress, the other reason is having example code for the users.

Users can refer to TOS hyp instead:

http://toshyp.atari.org/en/004013.html#Dbmsg

> IMHO the old C version was  more readable than the assembly
> one.  This API is so rarely used, that I don't think it matters
> though. :-)

I did this rewrite to assembler due to the limitations of the "--tos
none" mode (and since I did not want to put larger binaries into the
repository). More about that next weekend.

 Thomas