From c087539eb7fc0309eef94dda7e019595511f344a Mon Sep 17 00:00:00 2001 From: David Given Date: Wed, 28 Jul 2021 20:33:07 +0200 Subject: [PATCH 01/16] swap_sides isn't symmetrical for the encoder and the decoder. On the encoder, it affects which logical side is being written to which physical side. On the decoder, it modifies the logical side bit in the sector header itself. --- arch/ibm/decoder.cc | 2 +- arch/ibm/encoder.cc | 8 ++++---- arch/ibm/ibm.proto | 2 +- lib/decoders/decoders.cc | 10 +++++----- src/formats/atarist360.textpb | 2 +- src/formats/atarist370.textpb | 2 +- src/formats/atarist400.textpb | 2 +- src/formats/atarist410.textpb | 2 +- src/formats/atarist720.textpb | 2 +- src/formats/atarist740.textpb | 2 +- src/formats/atarist800.textpb | 2 +- src/formats/atarist820.textpb | 2 +- src/formats/commodore1581.textpb | 5 ++++- 13 files changed, 23 insertions(+), 20 deletions(-) diff --git a/arch/ibm/decoder.cc b/arch/ibm/decoder.cc index 971c80e9..b3f2bc9b 100644 --- a/arch/ibm/decoder.cc +++ b/arch/ibm/decoder.cc @@ -148,7 +148,7 @@ public: _sector->status = Sector::DATA_MISSING; /* correct but unintuitive */ if (_config.swap_sides()) - _sector->logicalSide = 1 - _sector->logicalSide; + _sector->logicalSide ^= 1; if (_config.ignore_side_byte()) _sector->logicalSide = _sector->physicalHead; } diff --git a/arch/ibm/encoder.cc b/arch/ibm/encoder.cc index 55e63947..932d6167 100644 --- a/arch/ibm/encoder.cc +++ b/arch/ibm/encoder.cc @@ -115,10 +115,11 @@ public: IbmEncoderProto::TrackdataProto trackdata; getTrackFormat(trackdata, physicalTrack, physicalSide); + int logicalSide = physicalSide ^ trackdata.swap_sides(); for (char sectorChar : trackdata.sector_skew()) { int sectorId = charToInt(sectorChar); - const auto& sector = image.get(physicalTrack, physicalSide, sectorId); + const auto& sector = image.get(physicalTrack, logicalSide, sectorId); if (sector) sectors.push_back(sector); } @@ -147,8 +148,6 @@ public: writeBytes(bytes); }; - if (trackdata.swap_sides()) - physicalSide = 1 - physicalSide; double clockRateUs = 1e3 / trackdata.clock_rate_khz(); if (!trackdata.use_fm()) clockRateUs /= 2.0; @@ -184,6 +183,7 @@ public: writeFillerBytes(trackdata.gap1(), gapFill); } + int logicalSide = physicalSide ^ trackdata.swap_sides(); bool first = true; for (char sectorChar : trackdata.sector_skew()) { @@ -192,7 +192,7 @@ public: writeFillerBytes(trackdata.gap3(), gapFill); first = false; - const auto& sectorData = image.get(physicalTrack, physicalSide, sectorId); + const auto& sectorData = image.get(physicalTrack, logicalSide, sectorId); if (!sectorData) { /* If there are any missing sectors, this is an empty track. */ diff --git a/arch/ibm/ibm.proto b/arch/ibm/ibm.proto index d76bcaa3..549f778b 100644 --- a/arch/ibm/ibm.proto +++ b/arch/ibm/ibm.proto @@ -6,7 +6,7 @@ message IbmDecoderProto { optional int32 sector_id_base = 1 [default = 1, (help) = "ID of first sector"]; optional bool ignore_side_byte = 2 [default = false, (help) = "ignore side byte in sector header"]; optional RangeProto required_sectors = 3 [(help) = "require these sectors to exist for a good read"]; - optional bool swap_sides = 4 [default = false, (help) = "swap side bytes when reading"]; + optional bool swap_sides = 4 [default = false, (help) = "put logical side 1 on physical side 0"]; } message IbmEncoderProto { diff --git a/lib/decoders/decoders.cc b/lib/decoders/decoders.cc index 356caae2..3f67c43a 100644 --- a/lib/decoders/decoders.cc +++ b/lib/decoders/decoders.cc @@ -59,12 +59,12 @@ std::unique_ptr AbstractDecoder::create(const DecoderProto& con } std::unique_ptr AbstractDecoder::decodeToSectors( - std::shared_ptr fluxmap, unsigned cylinder, unsigned head) + std::shared_ptr fluxmap, unsigned physicalCylinder, unsigned physicalHead) { _trackdata = std::make_unique(); _trackdata->fluxmap = fluxmap; - _trackdata->physicalCylinder = cylinder; - _trackdata->physicalHead = head; + _trackdata->physicalCylinder = physicalCylinder; + _trackdata->physicalHead = physicalHead; FluxmapReader fmr(*fluxmap); _fmr = &fmr; @@ -74,8 +74,8 @@ std::unique_ptr AbstractDecoder::decodeToSectors( { _sector = std::make_shared(); _sector->status = Sector::MISSING; - _sector->physicalCylinder = cylinder; - _sector->physicalHead = head; + _sector->physicalCylinder = physicalCylinder; + _sector->physicalHead = physicalHead; Fluxmap::Position recordStart = fmr.tell(); RecordType r = advanceToNextRecord(); diff --git a/src/formats/atarist360.textpb b/src/formats/atarist360.textpb index da33b12d..f2890366 100644 --- a/src/formats/atarist360.textpb +++ b/src/formats/atarist360.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/atarist370.textpb b/src/formats/atarist370.textpb index c9006ff3..7fd624de 100644 --- a/src/formats/atarist370.textpb +++ b/src/formats/atarist370.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/atarist400.textpb b/src/formats/atarist400.textpb index c86e9f2b..890b6686 100644 --- a/src/formats/atarist400.textpb +++ b/src/formats/atarist400.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/atarist410.textpb b/src/formats/atarist410.textpb index d09d8fd8..6142d54f 100644 --- a/src/formats/atarist410.textpb +++ b/src/formats/atarist410.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/atarist720.textpb b/src/formats/atarist720.textpb index f5446dba..59961cac 100644 --- a/src/formats/atarist720.textpb +++ b/src/formats/atarist720.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/atarist740.textpb b/src/formats/atarist740.textpb index 12a9fbed..1f02ce91 100644 --- a/src/formats/atarist740.textpb +++ b/src/formats/atarist740.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/atarist800.textpb b/src/formats/atarist800.textpb index 5e4edec4..ec54e3e5 100644 --- a/src/formats/atarist800.textpb +++ b/src/formats/atarist800.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/atarist820.textpb b/src/formats/atarist820.textpb index d86e7fee..e3354b59 100644 --- a/src/formats/atarist820.textpb +++ b/src/formats/atarist820.textpb @@ -34,7 +34,7 @@ encoder { decoder { ibm { - swap_sides: true + swap_sides: false } } diff --git a/src/formats/commodore1581.textpb b/src/formats/commodore1581.textpb index 794e0054..2251ef46 100644 --- a/src/formats/commodore1581.textpb +++ b/src/formats/commodore1581.textpb @@ -34,12 +34,15 @@ encoder { gap2: 22 gap3: 34 sector_skew: "0123456789" + swap_sides: true } } } decoder { - ibm {} + ibm { + swap_sides: false + } } cylinders { From 3ee88adfa9386976d9756212e29cac067f6af512 Mon Sep 17 00:00:00 2001 From: David Given Date: Fri, 30 Jul 2021 00:10:54 +0200 Subject: [PATCH 02/16] Add a simple tool for doing round-trip encode/decode tests. --- scripts/encodedecodetest.sh | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100755 scripts/encodedecodetest.sh diff --git a/scripts/encodedecodetest.sh b/scripts/encodedecodetest.sh new file mode 100755 index 00000000..4e88bb45 --- /dev/null +++ b/scripts/encodedecodetest.sh @@ -0,0 +1,26 @@ +#!/bin/sh +set -e +tmp=/tmp/$$ +srcfile=$tmp.src.img +fluxfile=$tmp.flux +destfile=$tmp.dest.img +format=$1 + +trap "rm -f $srcfile $fluxfile $destfile" EXIT + +dd if=/dev/urandom of=$srcfile bs=1M count=2 + +./fluxengine write $format -i $srcfile -d $fluxfile +./fluxengine read $format -s $fluxfile -o $destfile +if [ ! -s $destfile ]; then + echo "Zero length output file!" + exit 1 +fi + +truncate $srcfile -r $destfile +if ! cmp $srcfile $destfile; then + echo "Comparison failed!" + exit 1 +fi +exit 0 + From 7ab1288424e2127b7337f902c76dab0a81a76625 Mon Sep 17 00:00:00 2001 From: David Given Date: Sat, 31 Jul 2021 00:37:55 +0200 Subject: [PATCH 03/16] Pretty sure the Atari formats don't need swap_sides at all. --- src/formats/atarist360.textpb | 6 ++---- src/formats/atarist370.textpb | 5 +---- src/formats/atarist400.textpb | 5 +---- src/formats/atarist410.textpb | 5 +---- src/formats/atarist720.textpb | 5 +---- src/formats/atarist740.textpb | 5 +---- src/formats/atarist800.textpb | 5 +---- src/formats/atarist820.textpb | 5 +---- 8 files changed, 9 insertions(+), 32 deletions(-) diff --git a/src/formats/atarist360.textpb b/src/formats/atarist360.textpb index da33b12d..fb0aa564 100644 --- a/src/formats/atarist360.textpb +++ b/src/formats/atarist360.textpb @@ -27,15 +27,13 @@ encoder { gap2: 22 gap3: 34 sector_skew: "012345678" - swap_sides: true + sector_size: 512 } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { diff --git a/src/formats/atarist370.textpb b/src/formats/atarist370.textpb index c9006ff3..9f640d4c 100644 --- a/src/formats/atarist370.textpb +++ b/src/formats/atarist370.textpb @@ -27,15 +27,12 @@ encoder { gap2: 22 gap3: 34 sector_skew: "012345678" - swap_sides: true } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { diff --git a/src/formats/atarist400.textpb b/src/formats/atarist400.textpb index c86e9f2b..6d48232a 100644 --- a/src/formats/atarist400.textpb +++ b/src/formats/atarist400.textpb @@ -27,15 +27,12 @@ encoder { gap2: 22 gap3: 34 sector_skew: "0123456789" - swap_sides: true } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { diff --git a/src/formats/atarist410.textpb b/src/formats/atarist410.textpb index d09d8fd8..1aa93bb9 100644 --- a/src/formats/atarist410.textpb +++ b/src/formats/atarist410.textpb @@ -27,15 +27,12 @@ encoder { gap2: 22 gap3: 34 sector_skew: "0123456789" - swap_sides: true } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { diff --git a/src/formats/atarist720.textpb b/src/formats/atarist720.textpb index f5446dba..ebb2f06f 100644 --- a/src/formats/atarist720.textpb +++ b/src/formats/atarist720.textpb @@ -27,15 +27,12 @@ encoder { gap2: 22 gap3: 34 sector_skew: "012345678" - swap_sides: true } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { diff --git a/src/formats/atarist740.textpb b/src/formats/atarist740.textpb index 12a9fbed..88842e3b 100644 --- a/src/formats/atarist740.textpb +++ b/src/formats/atarist740.textpb @@ -27,15 +27,12 @@ encoder { gap2: 22 gap3: 34 sector_skew: "012345678" - swap_sides: true } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { diff --git a/src/formats/atarist800.textpb b/src/formats/atarist800.textpb index 5e4edec4..5cfd83ce 100644 --- a/src/formats/atarist800.textpb +++ b/src/formats/atarist800.textpb @@ -27,15 +27,12 @@ encoder { gap2: 22 gap3: 34 sector_skew: "0123456789" - swap_sides: true } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { diff --git a/src/formats/atarist820.textpb b/src/formats/atarist820.textpb index d86e7fee..4a5c8418 100644 --- a/src/formats/atarist820.textpb +++ b/src/formats/atarist820.textpb @@ -27,15 +27,12 @@ encoder { gap2: 22 gap3: 34 sector_skew: "0123456789" - swap_sides: true } } } decoder { - ibm { - swap_sides: true - } + ibm {} } cylinders { From c81c1926c01dc271e4660c05e0d8eca4fe792a40 Mon Sep 17 00:00:00 2001 From: David Given Date: Sat, 31 Jul 2021 00:38:13 +0200 Subject: [PATCH 04/16] Wire up the encode/decode tests. Hey, look, failures! --- Makefile | 2 +- arch/ibm/encoder.cc | 2 ++ mkninja.sh | 53 +++++++++++++++++++++++++------------ scripts/encodedecodetest.sh | 6 ++--- 4 files changed, 42 insertions(+), 21 deletions(-) diff --git a/Makefile b/Makefile index 4ea0328c..2f9e806f 100644 --- a/Makefile +++ b/Makefile @@ -50,7 +50,7 @@ CFLAGS += -Ilib -Idep/fmt -Iarch export OBJDIR = .obj all: .obj/build.ninja - @ninja -f .obj/build.ninja + @ninja -f .obj/build.ninja -v @if command -v cscope > /dev/null; then cscope -bRq; fi clean: diff --git a/arch/ibm/encoder.cc b/arch/ibm/encoder.cc index 55e63947..fef40622 100644 --- a/arch/ibm/encoder.cc +++ b/arch/ibm/encoder.cc @@ -115,6 +115,8 @@ public: IbmEncoderProto::TrackdataProto trackdata; getTrackFormat(trackdata, physicalTrack, physicalSide); + if (trackdata.swap_sides()) + physicalSide = 1 - physicalSide; for (char sectorChar : trackdata.sector_skew()) { int sectorId = charToInt(sectorChar); diff --git a/mkninja.sh b/mkninja.sh index 2470e77f..026dcdde 100644 --- a/mkninja.sh +++ b/mkninja.sh @@ -35,6 +35,10 @@ rule test command = \$in && touch \$out description = TEST \$in +rule encodedecode + command = sh scripts/encodedecodetest.sh \$format > \$out + description = ENCODEDECODE \$format + rule strip command = cp -f \$in \$out && $STRIP \$out description = STRIP \$in @@ -255,6 +259,14 @@ runtest() { echo build $OBJDIR/$prog.stamp : test $OBJDIR/$prog-debug$EXTENSION } +encodedecodetest() { + local format + format=$1 + + echo "build $OBJDIR/$format.encodedecode.stamp : encodedecode | fluxengine scripts/encodedecodetest.sh" + echo " format=$format" +} + buildlibrary libagg.a \ -Idep/agg/include \ dep/stb/stb_image_write.c \ @@ -374,13 +386,8 @@ buildlibrary libbackend.a \ lib/utils.cc \ lib/writer.cc \ -FORMATS="\ - acornadfs \ - acorndfs \ - aeslanier \ +RWFORMATS="\ amiga \ - ampro \ - apple2 \ atarist360 \ atarist370 \ atarist400 \ @@ -393,41 +400,49 @@ FORMATS="\ brother240 \ commodore1541 \ commodore1581 \ - eco1 \ - f85 \ - fb100 \ - hplif770 \ - ibm \ ibm1200_525 \ ibm1440 \ ibm180_525 \ ibm360_525 \ ibm720 \ ibm720_525 \ - mac400 \ mac800 \ - micropolis \ - mx \ northstar87 \ northstar175 \ northstar350 \ tids990 \ + " + +ROFORMATS="\ + acornadfs \ + acorndfs \ + aeslanier \ + ampro \ + apple2 \ + eco1 \ + f85 \ + fb100 \ + hplif770 \ + ibm \ + mac400 \ + micropolis \ + mx \ victor9k \ zilogmcz \ " -for pb in $FORMATS; do +for pb in $RWFORMATS $ROFORMATS; do buildencodedproto $OBJDIR/proto/libconfig.def ConfigProto \ formats_${pb}_pb src/formats/$pb.textpb $OBJDIR/proto/src/formats/$pb.cc done -buildmktable formats $OBJDIR/formats.cc $FORMATS +buildmktable formats $OBJDIR/formats.cc $RWFORMATS $ROFORMATS buildlibrary libfrontend.a \ -I$OBJDIR/proto \ -d $OBJDIR/proto/libconfig.def \ -d $OBJDIR/proto/libdata.def \ - $(for a in $FORMATS; do echo $OBJDIR/proto/src/formats/$a.cc; done) \ + $(for a in $RWFORMATS $ROFORMATS; do echo $OBJDIR/proto/src/formats/$a.cc; done) \ $OBJDIR/formats.cc \ src/fe-analysedriveresponse.cc \ src/fe-analyselayout.cc \ @@ -492,5 +507,9 @@ runtest proto-test -I$OBJDIR/proto \ tests/proto.cc \ $OBJDIR/proto/tests/testproto.cc +for f in $RWFORMATS; do + encodedecodetest $f +done + # vim: sw=4 ts=4 et diff --git a/scripts/encodedecodetest.sh b/scripts/encodedecodetest.sh index 4e88bb45..97fecbba 100755 --- a/scripts/encodedecodetest.sh +++ b/scripts/encodedecodetest.sh @@ -8,18 +8,18 @@ format=$1 trap "rm -f $srcfile $fluxfile $destfile" EXIT -dd if=/dev/urandom of=$srcfile bs=1M count=2 +dd if=/dev/urandom of=$srcfile bs=1M count=2 2>1 ./fluxengine write $format -i $srcfile -d $fluxfile ./fluxengine read $format -s $fluxfile -o $destfile if [ ! -s $destfile ]; then - echo "Zero length output file!" + echo "Zero length output file!" >&2 exit 1 fi truncate $srcfile -r $destfile if ! cmp $srcfile $destfile; then - echo "Comparison failed!" + echo "Comparison failed!" >&2 exit 1 fi exit 0 From 8df7998a8301a1e421253f55c5e9a8226bdc4f14 Mon Sep 17 00:00:00 2001 From: David Given Date: Sat, 31 Jul 2021 00:41:46 +0200 Subject: [PATCH 05/16] Don't use ninja -v. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2f9e806f..4ea0328c 100644 --- a/Makefile +++ b/Makefile @@ -50,7 +50,7 @@ CFLAGS += -Ilib -Idep/fmt -Iarch export OBJDIR = .obj all: .obj/build.ninja - @ninja -f .obj/build.ninja -v + @ninja -f .obj/build.ninja @if command -v cscope > /dev/null; then cscope -bRq; fi clean: From 9fa631acca628ace5ee5ff0e071f826baa7f34fd Mon Sep 17 00:00:00 2001 From: David Given Date: Sat, 31 Jul 2021 00:42:25 +0200 Subject: [PATCH 06/16] Fix the Brother encoder. --- arch/brother/encoder.cc | 41 +++++++---------------------------------- 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/arch/brother/encoder.cc b/arch/brother/encoder.cc index ed827c42..1d831987 100644 --- a/arch/brother/encoder.cc +++ b/arch/brother/encoder.cc @@ -8,33 +8,6 @@ #include "arch/brother/brother.pb.h" #include "lib/encoders/encoders.pb.h" -FlagGroup brotherEncoderFlags; - -static DoubleFlag clockRateUs( - { "--clock-rate" }, - "Encoded data clock rate (microseconds).", - 3.83); - -static DoubleFlag postIndexGapMs( - { "--post-index-gap" }, - "Post-index gap before first sector header (milliseconds).", - 1.0); - -static DoubleFlag sectorSpacingMs( - { "--sector-spacing" }, - "Time between successive sector headers (milliseconds).", - 16.2); - -static DoubleFlag postHeaderSpacingMs( - { "--post-header-spacing" }, - "Time between a sector's header and data records (milliseconds).", - 0.69); - -static StringFlag sectorSkew( - { "--sector-skew" }, - "Order in which to write sectors.", - "05a3816b4927"); - static int encode_header_gcr(uint16_t word) { switch (word) @@ -193,18 +166,18 @@ public: break; } - int bitsPerRevolution = 200000.0 / clockRateUs; - const std::string& skew = sectorSkew.get(); + int bitsPerRevolution = 200000.0 / _config.clock_rate_us(); + const std::string& skew = _config.sector_skew(); std::vector bits(bitsPerRevolution); unsigned cursor = 0; for (int sectorCount=0; sectorCount fluxmap(new Fluxmap); - fluxmap->appendBits(bits, clockRateUs*1e3); + fluxmap->appendBits(bits, _config.clock_rate_us()*1e3); return fluxmap; } From 9a12b651f9c41b9663e57909182d8dad31fce9c6 Mon Sep 17 00:00:00 2001 From: David Given Date: Sat, 31 Jul 2021 00:44:07 +0200 Subject: [PATCH 07/16] Remember to wire up the tids990 encoder. --- arch/tids990/tids990.h | 1 + lib/encoders/encoders.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/tids990/tids990.h b/arch/tids990/tids990.h index 5f0a9601..da1d2187 100644 --- a/arch/tids990/tids990.h +++ b/arch/tids990/tids990.h @@ -11,6 +11,7 @@ class DecoderProto; class EncoderProto; extern std::unique_ptr createTids990Decoder(const DecoderProto& config); +extern std::unique_ptr createTids990Encoder(const EncoderProto& config); #endif diff --git a/lib/encoders/encoders.cc b/lib/encoders/encoders.cc index bc091158..30a4486b 100644 --- a/lib/encoders/encoders.cc +++ b/lib/encoders/encoders.cc @@ -23,6 +23,7 @@ std::unique_ptr AbstractEncoder::create(const EncoderProto& con { EncoderProto::kIbm, createIbmEncoder }, { EncoderProto::kMacintosh, createMacintoshEncoder }, { EncoderProto::kNorthstar, createNorthstarEncoder }, + { EncoderProto::kTids990, createTids990Encoder }, }; auto encoder = encoders.find(config.format_case()); From a2911a95853998dbe08c76e2471b613b4c1a4fbc Mon Sep 17 00:00:00 2001 From: David Given Date: Sat, 31 Jul 2021 14:24:49 +0200 Subject: [PATCH 08/16] Allow setting oneof message fields to the default value. --- lib/proto.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/proto.cc b/lib/proto.cc index 29765800..4565c16e 100644 --- a/lib/proto.cc +++ b/lib/proto.cc @@ -162,6 +162,11 @@ void setProtoFieldFromString(ProtoField& protoField, const std::string& value) setRange((RangeProto*)reflection->MutableMessage(message, field), value); break; } + if (field->real_containing_oneof() && value.empty()) + { + reflection->MutableMessage(message, field); + break; + } /* fall through */ default: Error() << "can't set this config value type"; From 86d49d563e2aef481450ea74c3ebf27823ee8242 Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 13:30:26 +0200 Subject: [PATCH 09/16] Write the correct values to the sector headers! --- arch/macintosh/encoder.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/macintosh/encoder.cc b/arch/macintosh/encoder.cc index 8be1e054..e85421ea 100644 --- a/arch/macintosh/encoder.cc +++ b/arch/macintosh/encoder.cc @@ -174,9 +174,9 @@ static void write_sector(std::vector& bits, unsigned& cursor, const std::s write_bits(bits, cursor, 0xff3fcff3fcffLL, 6*8); /* sync */ write_bits(bits, cursor, MAC_SECTOR_RECORD, 3*8); - uint8_t encodedTrack = sector->physicalHead & 0x3f; + uint8_t encodedTrack = sector->logicalTrack & 0x3f; uint8_t encodedSector = sector->logicalSector; - uint8_t encodedSide = encode_side(sector->physicalCylinder, sector->logicalSide); + uint8_t encodedSide = encode_side(sector->logicalTrack, sector->logicalSide); uint8_t formatByte = MAC_FORMAT_BYTE; uint8_t headerChecksum = (encodedTrack ^ encodedSector ^ encodedSide ^ formatByte) & 0x3f; @@ -240,7 +240,7 @@ public: fillBitmapTo(bits, cursor, _config.post_index_gap_us() / clockRateUs, { true, false }); lastBit = false; - for (const auto& sector : collectSectors(physicalTrack, physicalSide, image)) + for (const auto& sector : sectors) write_sector(bits, cursor, sector); if (cursor >= bits.size()) From 516d43d7a8aeb1c2e7a81ad442028fc1ba4d1eba Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 13:56:31 +0200 Subject: [PATCH 10/16] Correctly parse extra config textpbs as textpbs, not binarypbs. --- lib/flags.cc | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/flags.cc b/lib/flags.cc index 6f6f8ff5..0c91ed04 100644 --- a/lib/flags.cc +++ b/lib/flags.cc @@ -183,9 +183,13 @@ void FlagGroup::parseFlagsWithConfigFiles(int argc, const char* argv[], parseFlags(argc, argv, [&](const auto& filename) { const auto& it = configFiles.find(filename); - std::string data; if (it != configFiles.end()) - data = it->second; + { + ConfigProto newConfig; + if (!newConfig.ParseFromString(it->second)) + Error() << "couldn't load built-in config proto"; + config.MergeFrom(newConfig); + } else { std::ifstream f(filename, std::ios::out); @@ -194,13 +198,10 @@ void FlagGroup::parseFlagsWithConfigFiles(int argc, const char* argv[], std::ostringstream ss; ss << f.rdbuf(); - data = ss.str(); - } - ConfigProto newConfig; - if (!newConfig.ParseFromString(data)) - Error() << "couldn't load config proto"; - config.MergeFrom(newConfig); + if (!google::protobuf::TextFormat::MergeFromString(ss.str(), &config)) + Error() << "couldn't load external config proto"; + } return true; } From 44c51f124690bb2ce0c723b3f5ed217cf20670db Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 13:56:51 +0200 Subject: [PATCH 11/16] Don't crash when given a missing sector. --- arch/northstar/encoder.cc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/arch/northstar/encoder.cc b/arch/northstar/encoder.cc index 44059018..44c09d4f 100644 --- a/arch/northstar/encoder.cc +++ b/arch/northstar/encoder.cc @@ -131,11 +131,10 @@ public: int bitsPerRevolution = 100000; double clockRateUs = 4.00; - if ((physicalTrack < 0) || (physicalTrack >= 35)) + if ((physicalTrack < 0) || (physicalTrack >= 35) || sectors.empty()) return std::unique_ptr(); - const auto& sector = image.get(physicalTrack, physicalSide, 0); - + const auto& sector = *sectors.begin(); if (sector->data.size() == NORTHSTAR_PAYLOAD_SIZE_SD) { bitsPerRevolution /= 2; // FM } else { @@ -145,11 +144,8 @@ public: std::vector bits(bitsPerRevolution); unsigned cursor = 0; - for (int sectorId = 0; sectorId < 10; sectorId++) - { - const auto& sectorData = image.get(physicalTrack, physicalSide, sectorId); + for (const auto& sectorData : sectors) write_sector(bits, cursor, sectorData); - } if (cursor > bits.size()) Error() << "track data overrun"; From b7cca1b95b6048e399c821ac6af511add6360907 Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 13:57:35 +0200 Subject: [PATCH 12/16] Add up_to_track in the image reader/writer trackdata proto. Abstract out the routine which fetches the trackdata for more consistent (and correct) results. --- lib/imagereader/imagereader.cc | 17 +++++++++++++++++ lib/imagereader/imagereader.proto | 12 +++++++----- lib/imagereader/imagereaderimpl.h | 8 ++++++++ lib/imagereader/imgimagereader.cc | 23 ++++++----------------- lib/imagewriter/imgimagewriter.cc | 20 +++----------------- 5 files changed, 41 insertions(+), 39 deletions(-) create mode 100644 lib/imagereader/imagereaderimpl.h diff --git a/lib/imagereader/imagereader.cc b/lib/imagereader/imagereader.cc index 88749862..0cf3adc5 100644 --- a/lib/imagereader/imagereader.cc +++ b/lib/imagereader/imagereader.cc @@ -69,3 +69,20 @@ ImageReader::ImageReader(const ImageReaderProto& config): _config(config) {} +void getTrackFormat(const ImgInputOutputProto& config, + ImgInputOutputProto::TrackdataProto& trackdata, unsigned track, unsigned side) +{ + trackdata.Clear(); + for (const ImgInputOutputProto::TrackdataProto& f : config.trackdata()) + { + if (f.has_track() && f.has_up_to_track() && ((track < f.track()) || (track > f.up_to_track()))) + continue; + if (f.has_track() && !f.has_up_to_track() && (track != f.track())) + continue; + if (f.has_side() && (f.side() != side)) + continue; + + trackdata.MergeFrom(f); + } +} + diff --git a/lib/imagereader/imagereader.proto b/lib/imagereader/imagereader.proto index 31af843d..3e181bc6 100644 --- a/lib/imagereader/imagereader.proto +++ b/lib/imagereader/imagereader.proto @@ -3,17 +3,19 @@ syntax = "proto2"; import "lib/common.proto"; message ImgInputOutputProto { + // NEXT: 6 message TrackdataProto { - optional int32 track = 1 [(help) = "if present, this format only applies to this track"]; - optional int32 side = 2 [(help) = "if present, this format only applies to this side"]; + optional int32 track = 1 [(help) = "if present, this format only applies to this track"]; + optional int32 up_to_track = 5 [(help) = "if present, forms a range with track"]; + optional int32 side = 2 [(help) = "if present, this format only applies to this side"]; optional int32 sector_size = 3 [default=512, (help) = "number of bytes per sector"]; - optional int32 sectors = 4 [(help) = "number of sectors in this track"]; + optional int32 sectors = 4 [(help) = "number of sectors in this track"]; } repeated TrackdataProto trackdata = 4 [(help) = "per-track format information (repeatable)"]; - optional int32 tracks = 5 [default=80, (help) = "number of tracks in image"]; - optional int32 sides = 6 [default=2, (help) = "number of sides in image"]; + optional int32 tracks = 5 [default=0, (help) = "number of tracks in image"]; + optional int32 sides = 6 [default=0, (help) = "number of sides in image"]; optional int32 physical_offset = 7 [default=0, (help) = "logical:physical track offset"]; optional int32 physical_step = 8 [default=1, (help) = "logical:physical track step"]; } diff --git a/lib/imagereader/imagereaderimpl.h b/lib/imagereader/imagereaderimpl.h new file mode 100644 index 00000000..ed80cba7 --- /dev/null +++ b/lib/imagereader/imagereaderimpl.h @@ -0,0 +1,8 @@ +#ifndef IMAGEREADERIMPL_H +#define IMAGEREADERIMPL_H + +extern void getTrackFormat(const ImgInputOutputProto& config, + ImgInputOutputProto::TrackdataProto& trackdata, unsigned track, unsigned side); + +#endif + diff --git a/lib/imagereader/imgimagereader.cc b/lib/imagereader/imgimagereader.cc index b8b45cf1..89dadf99 100644 --- a/lib/imagereader/imgimagereader.cc +++ b/lib/imagereader/imgimagereader.cc @@ -4,6 +4,7 @@ #include "imagereader/imagereader.h" #include "image.h" #include "lib/config.pb.h" +#include "imagereader/imagereaderimpl.h" #include "fmt/format.h" #include #include @@ -22,6 +23,9 @@ public: if (!inputFile.is_open()) Error() << "cannot open input file"; + if (!_config.img().tracks() || !_config.img().sides()) + Error() << "IMG: bad configuration; did you remember to set the tracks, sides and trackdata fields?"; + Image image; int trackCount = 0; for (int track = 0; track < _config.img().tracks(); track++) @@ -33,7 +37,7 @@ public: for (int side = 0; side < _config.img().sides(); side++) { ImgInputOutputProto::TrackdataProto trackdata; - getTrackFormat(trackdata, track, side); + getTrackFormat(_config.img(), trackdata, track, side); for (int sectorId = 0; sectorId < trackdata.sectors(); sectorId++) { @@ -55,26 +59,11 @@ public: image.calculateSize(); const Geometry& geometry = image.getGeometry(); - std::cout << fmt::format("reading {} tracks, {} sides, {} kB total\n", + std::cout << fmt::format("IMG: read {} tracks, {} sides, {} kB total\n", geometry.numTracks, geometry.numSides, inputFile.tellg() / 1024); return image; } - -private: - void getTrackFormat(ImgInputOutputProto::TrackdataProto& trackdata, unsigned track, unsigned side) - { - trackdata.Clear(); - for (const ImgInputOutputProto::TrackdataProto& f : _config.img().trackdata()) - { - if (f.has_track() && (f.track() != track)) - continue; - if (f.has_side() && (f.side() != side)) - continue; - - trackdata.MergeFrom(f); - } - } }; std::unique_ptr ImageReader::createImgImageReader( diff --git a/lib/imagewriter/imgimagewriter.cc b/lib/imagewriter/imgimagewriter.cc index 9238c2b6..8d40e6a6 100644 --- a/lib/imagewriter/imgimagewriter.cc +++ b/lib/imagewriter/imgimagewriter.cc @@ -4,6 +4,7 @@ #include "imagewriter/imagewriter.h" #include "image.h" #include "lib/config.pb.h" +#include "imagereader/imagereaderimpl.h" #include "fmt/format.h" #include #include @@ -32,7 +33,7 @@ public: for (int side = 0; side < sides; side++) { ImgInputOutputProto::TrackdataProto trackdata; - getTrackFormat(trackdata, track, side); + getTrackFormat(_config.img(), trackdata, track, side); int numSectors = trackdata.has_sectors() ? trackdata.sectors() : geometry.numSectors; int sectorSize = trackdata.has_sector_size() ? trackdata.sector_size() : geometry.sectorSize; @@ -48,25 +49,10 @@ public: } } - std::cout << fmt::format("wrote {} tracks, {} sides, {} kB total\n", + std::cout << fmt::format("IMG: wrote {} tracks, {} sides, {} kB total\n", tracks, sides, outputFile.tellp() / 1024); } - -private: - void getTrackFormat(ImgInputOutputProto::TrackdataProto& trackdata, unsigned track, unsigned side) - { - trackdata.Clear(); - for (const ImgInputOutputProto::TrackdataProto& f : _config.img().trackdata()) - { - if (f.has_track() && (f.track() != track)) - continue; - if (f.has_side() && (f.side() != side)) - continue; - - trackdata.MergeFrom(f); - } - } }; std::unique_ptr ImageWriter::createImgImageWriter( From b0a5174c0a78a1e8ebcb2c33ed696f5bc30bf77b Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 14:58:46 +0200 Subject: [PATCH 13/16] Wire up the (available) tests. --- mkninja.sh | 72 +++++++++++++++++++------------ scripts/commodore1541_test.textpb | 8 ++++ scripts/encodedecodetest.sh | 5 ++- scripts/mac400_test.textpb | 70 ++++++++++++++++++++++++++++++ scripts/mac800_test.textpb | 70 ++++++++++++++++++++++++++++++ 5 files changed, 196 insertions(+), 29 deletions(-) create mode 100644 scripts/commodore1541_test.textpb create mode 100644 scripts/mac400_test.textpb create mode 100644 scripts/mac800_test.textpb diff --git a/mkninja.sh b/mkninja.sh index 026dcdde..58d09b0c 100644 --- a/mkninja.sh +++ b/mkninja.sh @@ -36,7 +36,7 @@ rule test description = TEST \$in rule encodedecode - command = sh scripts/encodedecodetest.sh \$format > \$out + command = sh scripts/encodedecodetest.sh \$format \$configs > \$out description = ENCODEDECODE \$format rule strip @@ -262,9 +262,11 @@ runtest() { encodedecodetest() { local format format=$1 + shift - echo "build $OBJDIR/$format.encodedecode.stamp : encodedecode | fluxengine scripts/encodedecodetest.sh" + echo "build $OBJDIR/$format.encodedecode.stamp : encodedecode | fluxengine scripts/encodedecodetest.sh $*" echo " format=$format" + echo " configs=$*" } buildlibrary libagg.a \ @@ -386,8 +388,13 @@ buildlibrary libbackend.a \ lib/utils.cc \ lib/writer.cc \ -RWFORMATS="\ +FORMATS="\ + acornadfs \ + acorndfs \ + aeslanier \ amiga \ + ampro \ + apple2 \ atarist360 \ atarist370 \ atarist400 \ @@ -400,49 +407,41 @@ RWFORMATS="\ brother240 \ commodore1541 \ commodore1581 \ + eco1 \ + f85 \ + fb100 \ + hplif770 \ + ibm \ ibm1200_525 \ ibm1440 \ ibm180_525 \ ibm360_525 \ ibm720 \ ibm720_525 \ - mac800 \ - northstar87 \ - northstar175 \ - northstar350 \ - tids990 \ - " - -ROFORMATS="\ - acornadfs \ - acorndfs \ - aeslanier \ - ampro \ - apple2 \ - eco1 \ - f85 \ - fb100 \ - hplif770 \ - ibm \ mac400 \ + mac800 \ micropolis \ mx \ + northstar175 \ + northstar350 \ + northstar87 \ + tids990 \ victor9k \ zilogmcz \ " -for pb in $RWFORMATS $ROFORMATS; do +for pb in $FORMATS; do buildencodedproto $OBJDIR/proto/libconfig.def ConfigProto \ formats_${pb}_pb src/formats/$pb.textpb $OBJDIR/proto/src/formats/$pb.cc done -buildmktable formats $OBJDIR/formats.cc $RWFORMATS $ROFORMATS +buildmktable formats $OBJDIR/formats.cc $FORMATS buildlibrary libfrontend.a \ -I$OBJDIR/proto \ -d $OBJDIR/proto/libconfig.def \ -d $OBJDIR/proto/libdata.def \ - $(for a in $RWFORMATS $ROFORMATS; do echo $OBJDIR/proto/src/formats/$a.cc; done) \ + $(for a in $FORMATS; do echo $OBJDIR/proto/src/formats/$a.cc; done) \ $OBJDIR/formats.cc \ src/fe-analysedriveresponse.cc \ src/fe-analyselayout.cc \ @@ -507,9 +506,28 @@ runtest proto-test -I$OBJDIR/proto \ tests/proto.cc \ $OBJDIR/proto/tests/testproto.cc -for f in $RWFORMATS; do - encodedecodetest $f -done +encodedecodetest amiga +encodedecodetest atarist360 +encodedecodetest atarist370 +encodedecodetest atarist400 +encodedecodetest atarist410 +encodedecodetest atarist720 +encodedecodetest atarist740 +encodedecodetest atarist800 +encodedecodetest atarist820 +encodedecodetest brother120 +encodedecodetest brother240 +encodedecodetest ibm1200_525 +encodedecodetest ibm1440 +encodedecodetest ibm180_525 +encodedecodetest ibm360_525 +encodedecodetest ibm720 +encodedecodetest ibm720_525 +encodedecodetest tids990 +encodedecodetest commodore1581 +encodedecodetest commodore1541 scripts/commodore1541_test.textpb +encodedecodetest mac400 scripts/mac400_test.textpb +encodedecodetest mac800 scripts/mac800_test.textpb # vim: sw=4 ts=4 et diff --git a/scripts/commodore1541_test.textpb b/scripts/commodore1541_test.textpb new file mode 100644 index 00000000..b5e9a6d9 --- /dev/null +++ b/scripts/commodore1541_test.textpb @@ -0,0 +1,8 @@ +image_reader { + d64 {} +} + +image_writer { + d64 {} +} + diff --git a/scripts/encodedecodetest.sh b/scripts/encodedecodetest.sh index 97fecbba..3431f129 100755 --- a/scripts/encodedecodetest.sh +++ b/scripts/encodedecodetest.sh @@ -5,13 +5,14 @@ srcfile=$tmp.src.img fluxfile=$tmp.flux destfile=$tmp.dest.img format=$1 +shift trap "rm -f $srcfile $fluxfile $destfile" EXIT dd if=/dev/urandom of=$srcfile bs=1M count=2 2>1 -./fluxengine write $format -i $srcfile -d $fluxfile -./fluxengine read $format -s $fluxfile -o $destfile +./fluxengine write $format -i $srcfile -d $fluxfile "$@" +./fluxengine read $format -s $fluxfile -o $destfile "$@" if [ ! -s $destfile ]; then echo "Zero length output file!" >&2 exit 1 diff --git a/scripts/mac400_test.textpb b/scripts/mac400_test.textpb new file mode 100644 index 00000000..015cec5e --- /dev/null +++ b/scripts/mac400_test.textpb @@ -0,0 +1,70 @@ +image_reader { + img { + tracks: 80 + sides: 1 + trackdata { + sector_size: 524 + } + trackdata { + track: 0 + up_to_track: 15 + sectors: 12 + } + trackdata { + track: 16 + up_to_track: 31 + sectors: 11 + } + trackdata { + track: 32 + up_to_track: 47 + sectors: 10 + } + trackdata { + track: 48 + up_to_track: 63 + sectors: 9 + } + trackdata { + track: 64 + up_to_track: 79 + sectors: 8 + } + } +} + +image_writer { + img { + tracks: 80 + sides: 1 + trackdata { + sector_size: 524 + } + trackdata { + track: 0 + up_to_track: 15 + sectors: 12 + } + trackdata { + track: 16 + up_to_track: 31 + sectors: 11 + } + trackdata { + track: 32 + up_to_track: 47 + sectors: 10 + } + trackdata { + track: 48 + up_to_track: 63 + sectors: 9 + } + trackdata { + track: 64 + up_to_track: 79 + sectors: 8 + } + } +} + diff --git a/scripts/mac800_test.textpb b/scripts/mac800_test.textpb new file mode 100644 index 00000000..09e615ff --- /dev/null +++ b/scripts/mac800_test.textpb @@ -0,0 +1,70 @@ +image_reader { + img { + tracks: 80 + sides: 2 + trackdata { + sector_size: 524 + } + trackdata { + track: 0 + up_to_track: 15 + sectors: 12 + } + trackdata { + track: 16 + up_to_track: 31 + sectors: 11 + } + trackdata { + track: 32 + up_to_track: 47 + sectors: 10 + } + trackdata { + track: 48 + up_to_track: 63 + sectors: 9 + } + trackdata { + track: 64 + up_to_track: 79 + sectors: 8 + } + } +} + +image_writer { + img { + tracks: 80 + sides: 2 + trackdata { + sector_size: 524 + } + trackdata { + track: 0 + up_to_track: 15 + sectors: 12 + } + trackdata { + track: 16 + up_to_track: 31 + sectors: 11 + } + trackdata { + track: 32 + up_to_track: 47 + sectors: 10 + } + trackdata { + track: 48 + up_to_track: 63 + sectors: 9 + } + trackdata { + track: 64 + up_to_track: 79 + sectors: 8 + } + } +} + From 8b1bcf21eefd153193f9c1d4c333af2141d91219 Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 15:29:15 +0200 Subject: [PATCH 14/16] Don't run the encodedecode tests on non-Linux platforms. --- scripts/encodedecodetest.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/scripts/encodedecodetest.sh b/scripts/encodedecodetest.sh index 3431f129..939ea020 100755 --- a/scripts/encodedecodetest.sh +++ b/scripts/encodedecodetest.sh @@ -1,5 +1,11 @@ #!/bin/sh set -e + +if [ $(uname) != "Linux" ]; then + echo "Skipping test as not on Linux" + exit 0 +fi + tmp=/tmp/$$ srcfile=$tmp.src.img fluxfile=$tmp.flux From 740eacc7ac5ee53db03255d8354a6347fd289edd Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 15:29:34 +0200 Subject: [PATCH 15/16] real_*_oneof() is too new for GI's proto library. --- lib/proto.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/proto.cc b/lib/proto.cc index 4565c16e..41d93b4c 100644 --- a/lib/proto.cc +++ b/lib/proto.cc @@ -162,7 +162,7 @@ void setProtoFieldFromString(ProtoField& protoField, const std::string& value) setRange((RangeProto*)reflection->MutableMessage(message, field), value); break; } - if (field->real_containing_oneof() && value.empty()) + if (field->containing_oneof() && value.empty()) { reflection->MutableMessage(message, field); break; From 7344ee4402680defd6c2e98683dfe56ea5a124b2 Mon Sep 17 00:00:00 2001 From: David Given Date: Sun, 1 Aug 2021 19:12:16 +0200 Subject: [PATCH 16/16] Fix binary extension on Windows. --- mkninja.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mkninja.sh b/mkninja.sh index 58d09b0c..69ce1eca 100644 --- a/mkninja.sh +++ b/mkninja.sh @@ -264,7 +264,7 @@ encodedecodetest() { format=$1 shift - echo "build $OBJDIR/$format.encodedecode.stamp : encodedecode | fluxengine scripts/encodedecodetest.sh $*" + echo "build $OBJDIR/$format.encodedecode.stamp : encodedecode | fluxengine$EXTENSION scripts/encodedecodetest.sh $*" echo " format=$format" echo " configs=$*" }