Adding TSX file format support. Main changes. #1077
Conversation
src/cassette/CassettePlayer.cc
Outdated
| e2.getMessage() + | ||
| "\" and also failed to insert TSX image: \"" + | ||
| e3.getMessage() + '\"'); | ||
| } |
There was a problem hiding this comment.
We are nesting too much try-catch statements here. Perhaps we should extract it to other functions:
void CassettePlayer::insertTape(const Filename& filename)
{
try { ... }
catch (MSXException& e) {
insertCasTape(filename);
}
}
void CassetePlayer::insertCasTape(const Filename& filename)
{
try {
// The code that try to load the CAS tape
} catch (MSXException& e) {
insertTsxTape(filename);
}
}
void CassetePlayer::insertTsxTape(const Filename& filename)
{
// Your code here
}There was a problem hiding this comment.
fixed not really your suggestion but removed nested try-catch
src/cassette/TsxImage.hh
Outdated
| uint8_t lastbyte; //Used bits in the last byte (other bits should be 0) {8} | ||
| Endian::L16 pausems; //Pause after this block (ms.) {1000} | ||
| Endian::L24 len; //Length of data that follow | ||
| byte data[0]; //Data as in .TAP files |
There was a problem hiding this comment.
The comments have a different alignment here in Github. That's likely because we are mixing spaces and tabs.
src/cassette/TsxImage.cc
Outdated
| } | ||
| } | ||
|
|
||
| void TsxImage::writePulse(uint32_t tstates) { |
There was a problem hiding this comment.
The rest of the file uses a new line before opening brace. Style is broken here.
src/cassette/TsxImage.cc
Outdated
| cliComm.printWarning("Block#27 Unsupported yet!"); | ||
| pos += 1; | ||
| } else | ||
| if (bid == 0x31) { //Message Block |
There was a problem hiding this comment.
All magic numbers used in this if statements could be declared as constants. It would be clearer, less error-prone and would make the comments pointless.
|
Thanks for the comments @apoloval |
|
Hi, I've reviewed your patch in more detail. I've attached an updated version (diff against openmsx/master). Feel free to pick (only) the parts you like. Summary of the most important changes:
The buffer-overrun errors are still TODO. If you want I can work on that later this week. I also agree it might be better to refuse to load TSX file with unknown block types instead of ignoring those blocks. So that also still needs to change. Wouter BTW: I should also still write a reply for the non-technical issues. I'll try to do that this evening. |
|
Here's an updated version of the TSX patch. The most important change compared to last version is error checking. This includes buffer overrun checks (e.g. read beyond end of file), TSX version check and several other consistency checks (for as far as I understand the TSX/TZX file format). As we discussed, unknown features in the TSX file now cause an error instead of a warning (and then being skipped). While working on this I found another portability problem: the use of zero-sized arrays at the end of a struct. This is a gcc-extension from before the time C99 had the 'flexible array' feature (this is an array without an explicit size at the end of a struct). Though while 'flexible arrays' are part of the C99 standard, they are unfortunately not yet part of C++. Gcc and clang support them anyway in C++ mode, but visual studio does not :(. Luckily it wasn't too difficult to work around. I think I also found/fixed some off-by-one bugs in the code. E.g. the text-length skipping code in writeBlock32() and a few header length calculations (e.g. in writeBlock35()). Because of the new buffer checks that code looks very different now, so you won't immediately see the bug fixes. But review especially that code more carefully (maybe I was wrong in 'fixing' this). I was afraid that all the extra buffer checks would complicate the code. But I think I found a way to add the checks and at the same time also simplify the code. I removed the 'id' field from all the 'BlockXX' structs. It wasn't really used anyway (just made sizeof() of the struct one byte bigger). Now the structs match better with the TZX documentation. I inlined some functions that were only called once and had a very specific function. E.g. writeByte4B() only makes sense when called from the writeBlock4B() function. This allowed to change some TsxReader member variables into local variables (e.g. pulsePilot4B). And very important: I tried to be careful while refactoring, but I didn't run any tests. So no guarantees I didn't break anything. @nataliapc: please review. The only changed file (since last patch) is TsxImage.cc. It removes more lines than it adds :-) Attached is the full diff against openmsx/master. |
|
@m9710797 @nataliapc so what's the status of this PR? can it be merged after fixing the conflicts? would be nice to have TSX support (after almost 4 years) since a lot of TSX files exists of which are no cas/wav files available. |
|
Hi sndpl, Personally I'm not very motivated to work on this. The current status is:
Anyway, this is only a VERY short summary. But I don't like to repeat the whole discussion. If after reading the comments in this PR and the comments in the specific TSX support feature request thread, you still have question, then feel free to ask. Specifically for emulator users (openMSX and others): it's not too difficult to convert tsx to wav. So you don't have to miss anything. Wouter |
|
@sndpl thanks for ask about this It is clear that the openMSX team is completely against adding TSX in any way. He even comments that a subset of the TSX format was proposed and rejected, I don't know who rejected it, probably themselves. What I'm not going to do is spend more time improving a pull request as long as I don't see some kind of interest from them. There is currently quite a lot of support for the TSX format and it's a pity that the openMSX team doesn't find it interesting. As an alternative you can use one of the openMSX forks with TSX support: https://github.com/imulilla/openMSX_TSXadv (frequently updated) |
|
|
Time to close without merge? |
@m9710797 can you comment where/when it was proposed (by you?) and where/when it was rejected by whom? Apparently there is confusion about this seemingly important point and that's a pity.
Except for the many files created, what kind of support do you mean? Can you give an overview? |
|
Hi, Here you can find 1.684 TSX Files: Here you can find the Retro Virtual Machine Emulator with TSX Support: https://www.retrovirtualmachine.org/ Here you find CPCEC another MSX Emulator with TSX Support: http://cngsoft.no-ip.org/cpcec.htm Here you can find CSW2CDT a complete Computer TAPE Tool Suite that by the way... supports the TSX Format. ;-) http://cngsoft.no-ip.org/csw2cdt.htm I'm afraid that you only have excuses and more excuses... At the very begining the excuse was that there weren't enough TSX Files... Later the excuse was that there weren't any Emulator with TSX Support... Now the excuses are that TSX is a complex Format, that there aren't Tools with TSX Files Support.... and bla bla bla... Of course the TSX Format is an ampliation, an adding of a new Format Block to the TZX Format in order to support the MSX Format. And of course you can convert a TSX File to a WAV File because TSX Format is perfectly defined and lets you obtain the best and more acuraccy and pure WAV you can reach from to any other Formats. ;-) TSX Format can be perfect and easily added to the Open MSX Emulator if you want. The fact is that there are a non Official Open MSX Fork that supports perfectly the TSX Files. And needles to say that if you were really interested to adding the TSX Format to your Emulator Naty and any other members of the TSX TEAM could kindly offers you any kind of support, info, code, explanatios, etc., etc., etc., All the Formats can live together ina a perfect harmony: ROM, CAS, TSX, WAV and Users can choose the Format they prefer to use any time. But if you reject the TSX Format outright you are preventing MSX Users from choosing the TSX Format. Gracias & Saludetes. ;-) |
|
Hi manolito74, You forgot to list the 'tsx' branch in the official openMSX git repo: https://github.com/openMSX/openMSX/tree/tsx I know the non-official openMSX port with TSX support very well. I helped to develop that. |
|
Hi @m9710797, thank you very much for your comment/observation. ;-) And thank you very mucho, of coure, for your help and contribution to that branc version with TSX Support! ;-) Thanks for believing and supportint the TSX Format. ,-) Gracias & Saludetes. ;-) |
This Pull Request addresses my proposal #1076.
The proposal is to support the file format TSX (TZX 1.20 based) that add a new TZX block (4B) to be able to add MSX data blocks (as a specific implementation of Kansas City Standard used in many other old 8 bits platforms).
Please consider the aplication.
High speed load:
The code (TsxImage.cc) have a constant to disable/enable a high speed load mode. When enabled the MSX blocks speed are fixed up to 3675 bauds and silences reduced to 100 msec. Turbo blocks can't be speed up.
TZX/TSX Blocks supported (main blocks used with a MSX):
10 (turbo), 11 (turbo), 12 (tone), 13 (pulses), 20 (silence), 30 (text), 32 (archive), 35 (custom info), 4B (KCS)
Main thread in forum (spanish):
http://www.zonadepruebas.com/viewtopic.php?f=4&t=5369
Public FTP Repository for TSX files:
host: opencomputer.ddns.net:21
user: anonymous
Files added:
src/cassette/TsxImage.hh: The CassetteImage definition for TSX files.
src/cassette/TsxImage.cc: The implementation for TsxImage class.
src/utils/uint24.hh: A 24 bits unsigned integer type used in TZX blocks.
Changes:
share/scripts/_osd_menu.tcl: Added *.tsx support from OSD Tape menu.
src/cassette/CassettePlayer.cc: Try to read the file like a TSX file if CAS read fails.
src/cassette/CassettePlayerCLI.cc: Some strings changed.
src/utils/endian.hh: Endianness support for new type uint24_t.