machine/am9513.cpp: wrapped logerror with LOGWARN macro #12533
Conversation
Elektraglide
commented
Jun 30, 2024
Elektraglide
commented
- fixed wrong logging happening too early
|
Just a quick note: for MAME, the PR title and any notes largely go into the whatsnew.txt as-is. So we like to follow a format for the PR titles, where you start with the changed filename and its parent directory. In this case "machine/am9513.cpp: wrapped logerror for warnings with LOGWARN macro" would follow convention. We can edit the title as necessary before applying, which I'm going to do here, but obviously we'd rather not besides fixing the occasional autocorrect rampage. :-) |
Elektraglide
changed the title
There was a problem hiding this comment.
Firstly, I think doing string pasting in the macro is just obfuscation. Secondly, I don’t think these things warrant SCREAMING warning text.
On top of that, I don’t think warning log messages should be hidden behind a compile-time option anyway. They’re things that shouldn’t be triggered by valid guest code if we understand the device properly. It’s worth having that stuff sent to the debugger log by default.
Combined with the other issues, I think this PR is fairly misguided.
| if (!BIT(data, 5)) | ||
| { | ||
| LOGMASKED(LOG_MODE, "Disarm Counter %d\n", c + 1); | ||
| disarm_counter(c); | ||
| } | ||
| if (!BIT(data, 6)) | ||
| { | ||
| LOGMASKED(LOG_MODE, "Save Counter %d\n", c + 1); | ||
| save_counter(c); | ||
| } |
There was a problem hiding this comment.
LOG_MODE is for logging the master mode control setup. It’s inappropriate to use here.
Also, there are already LOG_GENERAL messages in both disarm_counter and save_counter so these are unnecessary.
| command_write(data & 0x00ff); | ||
|
|
||
| // NB testing afterwards because this command may have been changing bus width | ||
| if ((data & 0x00ff) == 0x00e7) | ||
| logerror("8-bit data bus selected with 16-bit write\n"); | ||
| LOGWARN("8-bit data bus selected with 16-bit write\n"); | ||
| else if ((data & 0x00ff) == 0x00ef && !bus_is_16_bit()) | ||
| logerror("16-bit data bus selected\n"); | ||
|
|
||
| command_write(data & 0x00ff); | ||
| LOGWARN("16-bit data bus selected\n"); |
There was a problem hiding this comment.
I think this won’t work properly after the change – else if ((data & 0x00ff) == 0x00ef && !bus_is_16_bit()) won’t be true after calling command_write because the 16-bit mode will have selected.
