FDC, now with writing support

[giuliof]

No description provided.

[giomba]

As requested, here is my early review.
I see there are still a lot of TODOs, but overall it looks it is taking the right path.
I look forward seeing how you will manage all the errors.
A lot of comments regard naming and style, which I wrote to make the code more consistent with the emulator's style.
As a general rule, even if you don't feel like implementing the error handling everywhere, please at least add some explicit log, so that an user can clearly see that an unhandled error has been detected. In this way, there won't be a totally silent fail, and future problem hunting would be easier. (= problem hunting: I mainly expect to encounter errors which are not implemented yet, but at least the emulator will make it clear that warning: you should implement something here, so that we know where to look)
I am not very familiar with the way the FDC works, and I thank you for your contribution.
I'll also look forward seeing the test stuff with the CMake refactoring you have shown me during last call.

[giomba]

I think that this could be interesting to exploit for some "copy protection" mechanism, even if I doubt it was something commonly used when this computer was used originally.
👍 for returning -1, but I would also add a LOG_WARN line, just to make it more clear, in case someone stumbles upon such a situation while using the emulator.

[giuliof]

Will be done

[giomba]

Coming from an embedded background, long looks ambiguous to me, and makes me costantly wonder "how long is this long? Should I worry?" (especially in the next lines where there are a lot of casts to long).

To make the code more readable and hide the casts, I suggest to replace magic numbers (and casts) with macros, like this:

+#define STANDARD_SECTOR_SIZE (1024L)
+#define TRACK0_SECTOR_SIZE   (256L)
+
 static int floppy_write_buffer(uint8_t *buffer, uint8_t unit_number,
                                bool phy_head, uint8_t phy_track, bool head,
                                uint8_t track, uint8_t sector) {
@@ -173,19 +176,19 @@ static int floppy_write_buffer(uint8_t *buffer, uint8_t unit_number,
         if (sector > 15)
             return -1;
         // Compute byte offset to sector start
-        offset = (long)sector * 256;
+        offset = sector * TRACK0_SECTOR_SIZE;
     } else {
         if (sector > 5)
             return -1;
 
         // Compute byte offset to sector start
-        offset = (long)track * 1024 * 5 * 2;
-        offset += (long)head * 1024 * 5;
-        offset += (long)sector * 1024;
+        offset = track * STANDARD_SECTOR_SIZE * 5 * 2;
+        offset += head * STANDARD_SECTOR_SIZE * 5;
+        offset += sector * STANDARD_SECTOR_SIZE;
 
         // First track has a different format
-        offset -= (long)1024 * 5;
-        offset += (long)256 * 16;
+        offset -= STANDARD_SECTOR_SIZE * 5;
+        offset += TRACK0_SECTOR_SIZE * 16;
 
         len = 1024;
     }

Note: macro's names are just an hint: feel free to work on them if it feels right.

You could also use other defines for 5 and 16, so that you won't need additional comments because code is self explanatory.
Example:

     // Locate sector start
     if (track == 0 && head == 0) {
-        // First track has max 16 sectors
-        if (sector > 15)
+#define TRACK0_SECTORS_PER_TRACK 16
+        if (sector >= TRACK0_SECTORS_PER_TRACK)
             return -1;
         // Compute byte offset to sector start
         offset = (long)sector * 256;
@@ -184,7 +184,8 @@ static int floppy_write_buffer(uint8_t *buffer, uint8_t unit_number,
         offset += (long)sector * 1024;
 
         // First track has a different format
-        offset -= (long)1024 * 5;
+#define STANDARD_SECTORS_PER_TRACK 5
+        offset -= (long)1024 * STANDARD_SECTORS_PER_TRACK;
         offset += (long)256 * 16;
 
         len = 1024;

Again: names and location are just an hint.

Does this look reasonable to you?

[giuliof]

Will be done

[giomba]

fseek and fwrite not working would be very... interesting.
I suggest to also LOG_ERR something, in case this happens, because this looks more like an issue of the underlying operating system, rather than of this emulator.

[giuliof]

fseek, fread and fwrite should set errno and this can be converted to string, can it be used to give a more informative message?

[giomba]

Here we can negate the condition and perform an early return to save indentation space.

[giomba]

Every other variable uses lower_snake_case, I suggest to use it here too for consistency.

[giuliof]

Done, will be pushed soon

[giomba]

This comment may become obsolete in case the error is handled.
For now, shall we log something, just to raise the user's attention?

[giuliof]

Same as before

[giomba]

I know what this function does because you told me, but can you add some documentation?
Also, the buffer_update() and buffer_write_size() functions which are called here, aren't really very descriptive of what this function is doing.

[giomba]

In order to make this readable, would you consider using typedef to declare the callback type, and then define the function pointer?
Example:

-static int (*read_buffer_cb)(uint8_t *buffer, uint8_t unit_number,
-                             bool phy_head, uint8_t phy_track, bool head,
-                             uint8_t track, uint8_t sector) = NULL;
+typedef int (*floppy_read_buffer_t)(uint8_t *buffer, uint8_t unit_number,
+                                    bool phy_head, uint8_t phy_track, bool head,
+                                    uint8_t track, uint8_t sector);
+static floppy_read_buffer_t read_buffer_cb = NULL;

This suggestion is also valid for write_buffer_cb.

[giuliof]

Done, will be pushed soon.

[giomba]

Do we need a separate fdc_registers.h file to manage some circular dependency? It looks like we can put all these definitions inside fdc.h

[giuliof]

Initially, all the content of fdc_registers.h was inside fdc.c since it doesn't need to be exported.
With the introduction of the tests, I needed to share register addresses between core implementation and tests only, so I created a separate file for this.

[giomba]

If this needs to be "public" (ie. in an header file), please prefix it to keep the global namespace as clean as possible.
Example:

- #define ADDR_STATUS_REGISTER (0x00)
+ #define FDC_ADDR_STATUS_REGISTER (0x00)

The same suggestion applies to cmd_t (see below), the enum values and the CMD_* ones.

As a general rule, I would suggest to always use prefixed #defines and enums, even in "closed" compilation units (.c), so we won't have to think about this anymore. (unless adding a prefix makes the code much much harder to read)