Merge pull request #139 from davidgiven/sectors

davidgiven · web-flow · commit adbcb2cd315d · 2020-02-24T21:55:27.000+01:00
Add support for required sectors, drive autodetection and fix homing on 8" drives.
diff --git a/FluxEngine.cydsn/CortexM3/ARM_GCC_541/Release/FluxEngine.hex b/FluxEngine.cydsn/CortexM3/ARM_GCC_541/Release/FluxEngine.hex
diff --git a/FluxEngine.cydsn/main.c b/FluxEngine.cydsn/main.c
@@ -17,6 +17,9 @@
 #define STEP_TOWARDS0 1
 #define STEP_AWAYFROM0 0
 
+static bool drive0_present;
+static bool drive1_present;
+
 static volatile uint32_t clock = 0; /* ms */
 static volatile bool index_irq = false;
 
@@ -42,6 +45,8 @@ static volatile bool dma_underrun = false;
 #define DECLARE_REPLY_FRAME(STRUCT, TYPE) \
     STRUCT r = {.f = { .type = TYPE, .size = sizeof(STRUCT) }}
 
+static void stop_motor(void);
+
 static void system_timer_cb(void)
 {
     CyGlobalIntDisable;
@@ -104,7 +109,10 @@ static void print(const char* msg, ...)
 static void set_drive_flags(struct set_drive_frame* flags)
 {
     if (current_drive_flags.drive != flags->drive)
+    {
+        stop_motor();
         homed = false;
+    }
     
     current_drive_flags = *flags;
     DRIVESELECT_REG_Write(flags->drive ? 2 : 1); /* select drive 1 or 0 */
@@ -184,19 +192,19 @@ static void step(int dir)
     CyDelay(STEP_INTERVAL_TIME);
 }
 
-static void home(void)
+/* returns true if it looks like a drive is attached */
+static bool home(void)
 {
     for (int i=0; i<100; i++)
     {
         /* Don't keep stepping forever, because if a drive's
          * not connected bad things happen. */
         if (TRACK0_REG_Read())
-            break;
+            return true;
         step(STEP_TOWARDS0);
     }
     
-    /* Step to -1, which should be a nop, to reset the disk on disk change. */
-    step(STEP_TOWARDS0);
+    return false;
 }
 
 static void seek_to(int track)
@@ -761,6 +769,10 @@ static void cmd_erase(struct erase_frame* f)
 
 static void cmd_set_drive(struct set_drive_frame* f)
 {
+    if (drive0_present && !drive1_present)
+        f->drive = 0;
+    if (drive1_present && !drive0_present)
+        f->drive = 1;
     set_drive_flags(f);
     
     DECLARE_REPLY_FRAME(struct any_frame, F_FRAME_SET_DRIVE_REPLY);
@@ -930,6 +942,18 @@ int main(void)
     
     CyWdtStart(CYWDT_1024_TICKS, CYWDT_LPMODE_DISABLED);
     
+    current_drive_flags.drive = 0;
+    start_motor();
+    drive0_present = home();
+    stop_motor();
+    
+    current_drive_flags.drive = 1;
+    start_motor();
+    drive1_present = home();
+    stop_motor();
+    
+    print("drive 0: %s drive 1: %s", drive0_present ? "yes" : "no", drive1_present ? "yes" : "no");
+    
     /* UART_PutString("GO\r"); */
 
     for (;;)
diff --git a/arch/ibm/ibm.h b/arch/ibm/ibm.h
@@ -32,18 +32,24 @@ struct IbmIdam
 class IbmDecoder : public AbstractDecoder
 {
 public:
-    IbmDecoder(unsigned sectorBase, bool ignoreSideByte=false):
+    IbmDecoder(unsigned sectorBase, bool ignoreSideByte=false,
+			const std::set<unsigned> requiredSectors=std::set<unsigned>()):
         _sectorBase(sectorBase),
-        _ignoreSideByte(ignoreSideByte)
+        _ignoreSideByte(ignoreSideByte),
+		_requiredSectors(requiredSectors)
     {}
 
     RecordType advanceToNextRecord();
     void decodeSectorRecord();
     void decodeDataRecord();
 
+	std::set<unsigned> requiredSectors(Track& track) const
+	{ return _requiredSectors; }
+
 private:
     unsigned _sectorBase;
     bool _ignoreSideByte;
+	std::set<unsigned> _requiredSectors;
     unsigned _currentSectorSize;
     unsigned _currentHeaderLength;
 };
diff --git a/arch/macintosh/decoder.cc b/arch/macintosh/decoder.cc
@@ -184,3 +184,26 @@ void MacintoshDecoder::decodeDataRecord()
     _sector->data.clear();
     _sector->data.writer().append(userData.slice(12, 512)).append(userData.slice(0, 12));
 }
+
+std::set<unsigned> MacintoshDecoder::requiredSectors(Track& track) const
+{
+	int count;
+	if (track.physicalTrack < 16)
+		count = 12;
+	else if (track.physicalTrack < 32)
+		count = 11;
+	else if (track.physicalTrack < 48)
+		count = 10;
+	else if (track.physicalTrack < 64)
+		count = 9;
+	else
+		count = 8;
+
+	std::set<unsigned> sectors;
+	do
+		sectors.insert(count);
+	while (count--);
+	return sectors;
+}
+
+
diff --git a/arch/macintosh/macintosh.h b/arch/macintosh/macintosh.h
@@ -18,6 +18,8 @@ class MacintoshDecoder : public AbstractDecoder
     RecordType advanceToNextRecord();
     void decodeSectorRecord();
     void decodeDataRecord();
+
+	std::set<unsigned> requiredSectors(Track& track) const;
 };
 
 #endif
diff --git a/doc/building.md b/doc/building.md
@@ -225,12 +225,21 @@ You should now have a working board, so it's time to test it.
      are cheap and just have the 3.5" connectors. Some are _very_ cheap and
      have a single 3.5" connector, after the twist.)
      
-	 FluxEngine uses, sadly, non-standard disk numbering (there are reasons).
-	 Drive 0 is the one nearest the motherboard; that is, before the twist.
-	 Drive 1 is the one at the end of the cable; that is, after the twist.
-	 Drive 0 is the default. If you only have one drive, remember to plug the
-	 drive into the connector _before_ the twist. (Or you can tell the client
-	 to select drive 1 by using `-s :d=1`.)
+     If you have **two** drives, plug them into both connectors. FluxEngine,
+     sadly, non-standard disk numbering (there are reasons). Drive 0 is the
+     one nearest the motherboard; that is, before the twist. Drive 1 is the
+     one at the end of the cable; that is, after the twist. Drive 0 is the
+     default. You can tell the client to select drive 1 by using `-s :d=1`.
+
+     If you have **one** drive, you may plug it into _either_ connector.
+     FluxEngine will autodetect it and treat it as drive 0. However, you'll
+     get the most reliable electrical signal if you plug it in at the end of
+     the cable.
+
+     **A note on termination:** some 5.25" drives require jumper configuration
+     to tell them whether they're at the end of the cable or in the middle of
+     the cable. 3.5" drives don't, and my 5.25" drives don't, so I can't
+     advise there. Consult your drive datasheet for details.
 
   3. **Important.** Make sure that no disk you care about is in the drive.
 	 (Because if your wiring is wrong and a disk is inserted, you'll corrupt
diff --git a/doc/disk-ibm.md b/doc/disk-ibm.md
@@ -41,19 +41,28 @@ of the disk image will vary depending on the format.
 
 Configuration options you'll want include:
 
-  - `--sector-id-base`: specifies the ID of the first sector; this defaults
-    to 1. Some formats (like the Acorn ones) start at 0. This can't be
+  - `--ibm-sector-id-base=N`: specifies the ID of the first sector; this defaults
+	to 1. Some formats (like the Acorn ones) start at 0. This can't be
 	autodetected because FluxEngine can't distinguish between a disk which
 	starts at sector 1 and a disk which starts at sector 0 but all the sector
 	0s are missing.
 
-  - `--ignore-side-byte`: each sector header describes the location of the
+  - `--ibm-ignore-side-byte=true|false`: each sector header describes the location of the
 	sector: sector ID, track and side. Some formats use the wrong side ID, so
 	the sectors on side 1 are labelled as belonging to side 0. This causes
 	FluxEngine to see duplicate sectors (as it can't distinguish between the
 	two sides). This option tells FluxEngine to ignore the side byte completely
 	and use the physical side instead.
 
+  - `--ibm-required-sectors=range`: if you know how many sectors to expect per
+	track, you can improve reads by telling FluxEngine what to expect here. If
+	a track is read and a sector on this list is _not_ present, then FluxEngine
+	assumes the read failed and will retry. This avoids the situation where
+	FluxEngine can't tell the difference between a sector missing because it's
+	bad or a sector missing because it was never written in the first place. If
+	sectors are seen outside the range here, it will still be read. You can use
+	the same syntax as for track specifiers: e.g. `0-9`, `0,1,2,3`, etc.
+
 
 Writing disks
 -------------
diff --git a/lib/dataspec.cc b/lib/dataspec.cc
@@ -15,34 +15,31 @@ std::vector<std::string> DataSpec::split(
 {
     std::vector<std::string> ret;
 
-    size_t start = 0;
-    size_t end = 0;
-    size_t len = 0;
-    do
-    {
-        end = s.find(delimiter,start); 
-        len = end - start;
-        std::string token = s.substr(start, len);
-        ret.emplace_back( token );
-        start += len + delimiter.length();
-    }
-    while (end != std::string::npos);
+	if (!s.empty())
+	{
+		size_t start = 0;
+		size_t end = 0;
+		size_t len = 0;
+		do
+		{
+			end = s.find(delimiter,start); 
+			len = end - start;
+			std::string token = s.substr(start, len);
+			ret.emplace_back( token );
+			start += len + delimiter.length();
+		}
+		while (end != std::string::npos);
+	}
+
     return ret;
 }
 
-DataSpec::Modifier DataSpec::parseMod(const std::string& spec)
+std::set<unsigned> DataSpec::parseRange(const std::string& data)
 {
-    static const std::regex MOD_REGEX("([a-z]*)=([-x+0-9,]*)");
-    static const std::regex DATA_REGEX("([0-9]+)(?:(?:-([0-9]+))|(?:\\+([0-9]+)))?(?:x([0-9]+))?");
+	static const std::regex DATA_REGEX("([0-9]+)(?:(?:-([0-9]+))|(?:\\+([0-9]+)))?(?:x([0-9]+))?");
 
-    std::smatch match;
-    if (!std::regex_match(spec, match, MOD_REGEX))
-        Error() << "invalid data modifier syntax '" << spec << "'";
-    
-    Modifier m;
-    m.name = match[1];
-    m.source = spec;
-    for (auto& data : split(match[2], ","))
+	std::set<unsigned> result;
+    for (auto& data : split(data, ","))
     {
         int start = 0;
         int count = 1;
@@ -64,17 +61,32 @@ DataSpec::Modifier DataSpec::parseMod(const std::string& spec)
             Error() << "mod '" << data << "' specifies an illegal quantity";
 
         for (int i = start; i < (start+count); i += step)
-            m.data.insert(i);
+            result.insert(i);
     }
 
+    return result;
+}
+
+DataSpec::Modifier DataSpec::parseMod(const std::string& spec)
+{
+	static const std::regex MOD_REGEX("([a-z]*)=([-x+0-9,]*)");
+
+    std::smatch match;
+    if (!std::regex_match(spec, match, MOD_REGEX))
+        Error() << "invalid data modifier syntax '" << spec << "'";
+    
+    Modifier m;
+    m.name = match[1];
+    m.source = spec;
+	m.data = parseRange(match[2]);
     return m;
 }
 
 void DataSpec::set(const std::string& spec)
 {
     std::vector<std::string> words = split(spec, ":");
     if (words.size() == 0)
-        Error() << "empty data specification (you have to specify *something*)";
+		return;
 
     filename = words[0];
     if (words.size() > 1)
diff --git a/lib/dataspec.h b/lib/dataspec.h
@@ -34,6 +34,8 @@ class DataSpec
 public:
     static std::vector<std::string> split(
         const std::string& s, const std::string& delimiter);
+	static std::set<unsigned> parseRange(const std::string& spec);
+
     static Modifier parseMod(const std::string& spec);
 
 public:
@@ -117,4 +119,34 @@ class DataSpecFlag : public Flag
     DataSpec _value;
 };
 
+class RangeFlag : public Flag
+{
+public:
+    RangeFlag(const std::vector<std::string>& names, const std::string helptext,
+            const std::string& defaultValue):
+        Flag(names, helptext),
+		_stringValue(defaultValue),
+        _value(DataSpec::parseRange(defaultValue))
+    {}
+
+    const std::set<unsigned>& get() const
+    { checkInitialised(); return _value; }
+
+    operator const std::set<unsigned>& () const
+    { return get(); }
+
+    bool hasArgument() const { return true; }
+    const std::string defaultValueAsString() const { return _stringValue; }
+
+    void set(const std::string& value)
+	{
+		_stringValue = value;
+		_value = DataSpec::parseRange(value);
+	}
+
+private:
+	std::string _stringValue;
+    std::set<unsigned> _value;
+};
+
 #endif
diff --git a/lib/decoders/decoders.cc b/lib/decoders/decoders.cc
@@ -88,3 +88,10 @@ void AbstractDecoder::pushRecord(const Fluxmap::Position& start, const Fluxmap::
     _track->rawrecords.push_back(record);
     _fmr->seek(here);
 }
+
+std::set<unsigned> AbstractDecoder::requiredSectors(Track& track) const
+{
+	static std::set<unsigned> empty;
+	return empty;
+}
+
diff --git a/lib/decoders/decoders.h b/lib/decoders/decoders.h
@@ -52,6 +52,11 @@ class AbstractDecoder
     void seek(const Fluxmap::Position& pos)
     { return _fmr->seek(pos); } 
 
+	/* Returns a set of sectors required to exist on this track. If the reader
+	 * sees any missing, it will consider this to be an error and will retry
+	 * the read. */
+	virtual std::set<unsigned> requiredSectors(Track& track) const;
+
 protected:
     virtual void beginTrack() {};
     virtual RecordType advanceToNextRecord() = 0;
diff --git a/lib/reader.cc b/lib/reader.cc
diff --git a/lib/usb.cc b/lib/usb.cc
diff --git a/src/fe-readibm.cc b/src/fe-readibm.cc
diff --git a/tests/dataspec.cc b/tests/dataspec.cc
