Conversation
- Mapper is ASCII16 - Assumes FDD hardware in slot 3-2
ALLOC corrupts HL/DE via CLRFCB, so `ex de,hl` after the call stored garbage in SLTWRK instead of the work area base address. Fix: read `(HIMEM)` instead, since ALLOC lowers HIMEM to the base of the allocated area. Bug probably never manifested because no driver previously requested a work area.
- Ghost drive is assigned if exactly one device is reported as being an FDD by the driver. - The ghost drive is always the following of the main drive. - Only one ghost drive is assigned in the entire drives space. - The mechanism is transparent for drivers, the kernel handles everything. - In DOS 2 mode this is tracked via new flags in UD_DFLAGS and a new GHO_PAIR internal variable. - In DOS 1 mode a new "FDD" flag is added to UD1_RELATIVE_DRIVE, and the frist byte of the first sector field is repurposed to track main/ghost drive and last one accessed. - Code has been added to ROM bank 6, makefile is adjusted. - Includes optimization to doshead.mac (call bank 0 and 4 routines are now unified).
- _GDLI returns a new type byte for ghost drives - _MAPDRV unmaps ghost drive too when unmapping master FDD drive - _MAPDRV automaps the next drive as ghost when ampping an FDD drive if the next drive is free and no other ghost drive is mapped - DRVINFO.COM shows ghost drive information
- New driver query: get formatting choices for device
- Possible outputs: single choice, SD+DD, SD+DD+HD, custom string
to be copied to a buffer (with maximum length)
- New driver query: format disk
- Must set DOS 1 compatible boot sector, FAT and root directory
- Both queries implemented in the standalone ASCII 8 and Turbo-R FDD drivers
- _FORMAT function call:
- New operation code: 80h, will copy the choice string to a buffer;
works for Nextor FDD and MSX-DOS drives
- Operation code 0 now works with MSX-DOS drives only
- All other operation now work with Nextor FDD and MSX-DOS drives only
- CALL FORMAT adjusted accordingly, in both DOS 1 and DOS 2 modes
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (3)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can customize the tone of the review comments and chat replies.Configure the 📝 WalkthroughWalkthroughAdds phased floppy-disk support and ghost-drive infrastructure: floppy flags and UD layout changes, no‑partition behavior for floppies, media‑ID pass‑through on I/O, new device queries for formatting/motor control, ghost‑drive creation on init, bank‑6 format/ghost subsystems, and a Turbo‑R FDD driver plus build targets. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Bank0/DOSINIT
participant B2 as Bank2
participant B6 as Bank6(ghost)
participant DRVTBL as DRVTBL
participant UD as UNIT_TAB
Init->>B2: continue post‑autodrv
B2->>B6: CALBNK SETUP_GHOST_DOS2
B6->>DRVTBL: scan for single FDD
alt single FDD found and slot available
B6->>UD: allocate/shift UD, copy DPB
B6->>DRVTBL: insert ghost UD, set UF_FDD+UF_GHO
else no-op
B6->>B2: return
end
B6->>B2: return (ghost created or skipped)
B2->>Init: resume init
sequenceDiagram
participant User as User
participant B3 as Bank3/DOS1
participant B6F as Bank6/format
participant Drv as Driver
User->>B3: FORMAT command
B3->>B3: GET_FMT_MASK
B3->>User: request drive selection (if needed)
B3->>B6F: CALL F_FORMAT
B6F->>Drv: DEVICE_QUERY DEVQ_GET_FORMAT_CHOICES
alt driver provides choices
B6F->>User: display driver choices
else fallback
B6F->>User: build/display default choices
end
User->>B6F: select choice
B6F->>Drv: DEVQ_DO_FORMAT (or kernel DSK_FORMAT)
B6F->>B3: return result
sequenceDiagram
participant App as App
participant DIO as DIO_ENTRY
participant DRVTBL as DRVTBL
participant Ghost as GHOST_PROMPT
participant B0 as Bank0/PROMPT
participant Drv as Driver
App->>DIO: read/write to drive
DIO->>DRVTBL: fetch UD (ghost detected)
DIO->>DIO: check last‑accessed flag
alt ghost not last‑accessed
DIO->>Ghost: CALL GHOST_PROMPT
Ghost->>DRVTBL: set/clear last‑accessed bits
Ghost->>B0: CALL PROMPT (user action)
B0->>App: user confirms/changes media
end
DIO->>Drv: issue I/O with base sector=0 and C=media id
Drv->>DIO: operation completes
DIO->>App: return status/data
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
source/kernel/drivers/TurboRFDD/chgbnk.mac (1)
27-30: Stale/misleading comment: references Sunrise IDE bit-swapping, but this is a plain ASCII16 mapper.The comment describes Sunrise IDE's unusual bit-reversal behavior for bank switching, but the actual
CHGBNKimplementation (line 36) is a straightforwardld (6000h),afor the ASCII16 mapper. This looks like a copy-paste artifact from another driver'schgbnk.mac. Consider removing or replacing these lines to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/drivers/TurboRFDD/chgbnk.mac` around lines 27 - 30, The comment describing Sunrise IDE's bit-swapping for bank bits is stale and misleading for this file: the actual CHGBNK implementation uses a plain ASCII16 mapper (the code writes A to (6000h)), so remove or replace the Sunrise-specific bit-swapping notes with a short, accurate comment stating that CHGBNK performs a direct write to 0x6000 for ASCII16 bank switching (reference symbol: CHGBNK and the write to (6000h)). Ensure the new comment clearly identifies the mapper behavior and does not mention Sunrise/bit-reversal.source/kernel/drivers/TurboRFDD/driver.mac (2)
331-369: Stubs have extraretinstructions — intentional padding?Each stub (OEMSTAT, BASDEV, EXTBIO, DIRECT_0–4) contains extra
retinstructions beyond what's needed. For instance,OEMSTATat line 331–334 hasscf; ret; ret— the third byte is dead code. If this is intentional padding for alignment or future use, a brief comment would clarify intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/drivers/TurboRFDD/driver.mac` around lines 331 - 369, The stub routines OEMSTAT, BASDEV, EXTBIO, and DIRECT_0–DIRECT_4 contain redundant trailing `ret` instructions (e.g., `scf; ret; ret`) which are dead bytes; either remove the extra `ret` lines so each stub contains only the minimal return sequence, or if the extra `ret`s are intended as padding/alignment, add a brief comment above each label (e.g., "padding for alignment/expansion") clarifying the intent; update the labels OEMSTAT, BASDEV, EXTBIO, DIRECT_0, DIRECT_1, DIRECT_2, DIRECT_3, and DIRECT_4 accordingly.
1973-2014: Verify buffer address is in page 2/3 at runtime.The RAM transfer routine switches page 1 to slot 3-2, so any buffer in page 1 (4000h–7FFFh) would become inaccessible during the transfer. The comment at line 2025 notes the requirement, but there's no runtime validation. If callers can ever pass a page-1 buffer, this would silently corrupt data.
This is likely safe since the kernel ensures DTA/buffers are in page 2/3, but worth a defensive note.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/drivers/TurboRFDD/driver.mac` around lines 1973 - 2014, CALL_XFER must validate the buffer is not in page 1 before patching/switching pages: after DE is set to the buffer (the ex de,hl that yields "DE = buffer") add a runtime check on DE's high byte (D) to detect addresses in 4000h–7FFFh (page 1) and branch to a new handler (e.g., XFER_BAD_BUFFER) that returns an error or aborts the transfer instead of continuing; reference the CALL_XFER label, the DE/HL registers, and the page-switching sequence that uses WK_XFER/WK_FLAGS/XFER_PATCH_OFF so the guard runs before the push/pop/page-switching code and prevents the RAM routine jump (jp (iy)) when the buffer is in page 1.source/kernel/const.inc (1)
244-260: New flag definitions look correct.Bit assignments are unique and non-overlapping, masks are properly computed. One tiny style nit: line 255 uses uppercase
EQUwhile all other definitions in this section use lowercaseequ.Nitpick: consistent casing
-UFM_FD EQU (1 SHL UF_FDD) +UFM_FD equ (1 SHL UF_FDD)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/const.inc` around lines 244 - 260, The UFM_FD mask line uses uppercase "EQU" while the surrounding definitions use lowercase "equ"; change the directive for UFM_FD (the line defining UFM_FD / symbol UFM_FD or UF_FD context) to use lowercase "equ" to match the file's style so casing is consistent with the other mask definitions (e.g., UFM_DB, UFM_RM).source/kernel/Makefile (1)
67-67: Static analysis: declareallas.PHONY.
alldoesn't produce a file and should be declared phony to avoid issues if a file namedallever exists. Also note that line 71's.phony: prerequisitesuses lowercase, which GNU Make does not recognize — it should be.PHONY.Proposed fix
+.PHONY: all base ide ide-masteronly ide-blue ide-masteronly-emu ascii8 ascii16 mfrsd flashjacks ocm dsk-tr all: base ide ide-masteronly ide-blue ide-masteronly-emu ascii8 ascii16 mfrsd flashjacks ocm dsk-tr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/Makefile` at line 67, Declare the Make target all as phony and fix the incorrect lowercase phony declaration: add a .PHONY line that includes the all target (e.g., ".PHONY: all" alongside other phony targets) and replace any ".phony:" occurrences with the correct ".PHONY:" to ensure GNU Make treats these targets as phony.source/kernel/bank6/format.mac (2)
338-340: Redundant public declaration:publicdirective and::suffix both export the symbol.Line 338 declares
public ?FMT_PRINT_AND_CHOOSE, and line 340 uses::which also makes the label public. Only one is needed. The other entry points (?BUILD_FMT_CHOICES,?F_FORMAT) usepublicwithout::, so removing the::here would be consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/bank6/format.mac` around lines 338 - 340, The symbol ?FMT_PRINT_AND_CHOOSE is exported twice (via the `public ?FMT_PRINT_AND_CHOOSE` directive and the `?FMT_PRINT_AND_CHOOSE::` label); remove the redundant export by changing the label to a normal local label (remove the `::` suffix) and keep the existing `public ?FMT_PRINT_AND_CHOOSE` declaration so the symbol is exported consistently with other entry points like ?BUILD_FMT_CHOICES and ?F_FORMAT.
86-92: DRVTBL walk has no bounds check — unbounded loop if drive table is inconsistent.If
Bis within range but DRVTBL entry counts don't sum high enough (e.g. due to corruption or a drive-count mismatch), this loop will read past the 4-entry DRVTBL into unrelated memory.A 4-iteration safety counter (matching the 4 DRVTBL slots) would make this robust:
Proposed fix
+ ld e,4 ;Max 4 DRVTBL entries fformat_drvtbl: sub (hl) jr c,fformat_found_drv inc hl inc hl + dec e + jr z,fformat_pop_iform ;Safety: not found jr fformat_drvtbl🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/bank6/format.mac` around lines 86 - 92, The DRVTBL walker loop at label fformat_drvtbl can run unbounded if DRVTBL entries are inconsistent; add a 4-iteration safety counter (matching the 4 DRVTBL slots) before the loop (e.g., load a register with 4) and decrement it each iteration, branching to a new failure path (e.g., fformat_drvtbl_not_found) when it reaches zero; update the loop around the existing sub (hl), jr c,fformat_found_drv and inc hl/inc hl sequence so the counter is checked after each iteration to prevent reading past DRVTBL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ai/PLAN.md:
- Line 198: Typo in the heading "### Customizng the message": update the heading
text to "### Customizing the message" in the .ai/PLAN.md file so the word is
spelled correctly; locate the heading line containing "Customizng the message"
and replace it with "Customizing the message".
- Line 180: Typo fix: change "othe" to "the" in the sentence starting "Idea:
when Nextor detects..." in the PLAN entry so it reads "query information for all
the other devices"; locate that line referencing "floppy disk drive" / "ghost
drive" and correct the single-word spelling error.
- Line 150: Fix the typo in the PLAN description for the "quick format" command
by changing "rewritting" to "rewriting" in the sentence that describes wiping
floppy content and rewriting the boot sector, FAT and root directory (search for
the phrase "quick format" or the word "rewritting" in .ai/PLAN.md to locate the
line).
- Around line 27-37: Update the incorrect bit reference in the Flags
description: change the sentence that says the floppy disk drive identification
is "covered by the flag in bit 3" to refer to "bit 2" instead, so it matches the
spec lines listing "bit 2: 1 if the device is a floppy disk drive"; locate the
paragraph under the "Flags:" spec block and replace "bit 3" with "bit 2" to
correct the mismatch with the existing bit definitions.
In `@source/kernel/bank2/val.mac`:
- Around line 55-215: The ROM path can overrun the caller buffer because
dcb_add_dots always appends 7 bytes after the null position; modify the copy
loop (dcb_rom_loop / dcb_nxt_truncated path) to ensure you leave room for the
suffix (only copy up to max_size-6 bytes or stop early and avoid calling
dcb_add_dots if insufficient space) and adjust callers that set BC/DE
accordingly; additionally the Nextor path (dcb_nxt_build calling
BUILD_FMT_CHOICES via C4PBK## with BK4_ADD##) writes into SECBUF rather than the
caller IX buffer—after C4PBK returns, add a controlled copy routine that copies
from SECBUF into the IX buffer using the saved buffer size (pop/saved DE or BC)
and respect the same truncation/suffix rules (or have BUILD_FMT_CHOICES write
directly to IX) so no overflow occurs and the caller buffer receives the string.
In `@source/kernel/bank3/dos1ker.mac`:
- Around line 5943-5963: The RR C in label FMT_LIST_LOOP relies on an undefined
carry and causes phantom bits/infinite loop; fix by ensuring carry is cleared
before the loop or by replacing RR C with a logical shift. Specifically, either
insert an instruction that clears the carry (e.g., xor a or scf/ccf sequence as
appropriate) immediately before entering FMT_LIST_LOOP, or change the RR C at
FMT_LIST_LOOP to SRL C so bit7 is zero-filled; keep the rest of the loop (calls
to A408F, labels FMT_LIST_NOCOMMA, FMT_LIST_SKIP) unchanged.
In `@source/kernel/bank4/partit.mac`:
- Around line 4094-4099: The call to MAYBE_CREATE_GHOST_DOS1 unconditionally
marks the entry as a ghost (overwriting FAS byte 0 with 0x81) even after an
explicit MAPDRV first-absolute-sector was stored in the KERNEX entry, corrupting
that user value; update the MAPDRV/FDD path (the block using SECBUF, the FDD
test via bit 2,(iy+7), and the set 6,(ix+UD1_RELATIVE_DRIVE##) before calling
MAYBE_CREATE_GHOST_DOS1) so that either (a) you skip calling
MAYBE_CREATE_GHOST_DOS1 when an explicit first-absolute sector has been written,
or (b) modify MAYBE_CREATE_GHOST_DOS1 (and its helpers) to preserve FAS byte 0
when a user-specified sector is present (set the ghost flag in a different byte
or set FAS only after saving/restoring the explicit sector). Locate this logic
around the symbols SECBUF, UD1_RELATIVE_DRIVE##, MAYBE_CREATE_GHOST_DOS1 and the
KERNEX FAS storage and implement the guard or preservation accordingly.
- Around line 3838-3861: MAYBE_CREATE_GHOST_DOS1 currently clobbers the master's
UD1_FIRST_ABSOLUTE_SECTOR low byte and GHOST_CLEANUP_DOS1 only masks bits back,
which fails if the original low byte had bits 0,1 or 7 set; fix by preserving
the original byte when creating the ghost and restoring it exactly on cleanup:
modify MAYBE_CREATE_GHOST_DOS1 to load and store
(iy+UD1_FIRST_ABSOLUTE_SECTOR##) into a new per-entry slot (e.g.
UD1_SAVED_FAS_LO## or reuse an unused UD1 field) before writing 81h/02h, and
update GHOST_CLEANUP_DOS1 to restore that saved value instead of relying on an
AND mask; alternatively introduce a separate ghost-state field and stop
repurposing UD1_FIRST_ABSOLUTE_SECTOR (ensure MAPDOS1_NODEF still calls
MAYBE_CREATE_GHOST_DOS1 safely).
In `@source/kernel/drivers/StandaloneASCII8/driver.mac`:
- Around line 541-551: The comment for the device query DO_DEVQ_STOP_MOTOR
incorrectly documents QUERY_OK as "ok, disk has been formatted"; update the
comment block above DO_DEVQ_STOP_MOTOR so the QUERY_OK line correctly states
that the motor has been stopped (e.g., "QUERY_OK: ok, motor has been stopped"),
leaving QUERY_INVALID_DEVICE and QUERY_NOT_IMPLEMENTED descriptions unchanged;
locate the block containing DO_DEVQ_STOP_MOTOR and amend only the textual
documentation.
In `@source/kernel/drivers/TurboRFDD/driver.mac`:
- Around line 1038-1071: The boot template currently sets heads=2 for
single-sided 360K disks by embedding the sequence starting at
BOOT_DATA/BOOT_PARMS_OFF that places 02h into the BPB heads field; update
BOOT_PARMS_360K so the heads byte is 01h (set number-of-heads = 1) to reflect
SS/DD 360K geometry, or if 02h is intentional for legacy compatibility, add a
clarifying comment next to BOOT_DATA/BOOT_PARMS_360K explaining the deliberate
choice.
- Around line 1625-1632: FDC_SENSE_INT currently ignores the carry set by
FDC_WRITE_CMD and always calls FDC_READ_RESULT, risking stale reads and
downstream hangs (e.g., FDC_WAIT_SEEK). After the call to FDC_WRITE_CMD in
FDC_SENSE_INT, test the carry flag and, if set, abort the routine (restore
registers and return with carry set) instead of calling FDC_READ_RESULT; only
call FDC_READ_RESULT when the write succeeded. Update FDC_SENSE_INT to restore
bc/ix correctly on the error path so the caller can detect the timeout via the
carry flag.
- Around line 1606-1620: The FDC_WAIT_SEEK sequence (labels FDC_WAIT_SEEK /
FWS_BSY and FWS_CHK) currently busy-waits with no timeout; add an
iteration-count timeout like FDC_WAIT_READY and FDC_WRITE_CMD: introduce a small
counter (e.g., in a register or IX+offset) before entering FWS_BSY and decrement
it on each loop, and if it reaches zero jump to a timeout handler that sets an
error condition (set carry/return non-zero or set a status byte) and returns; do
the same for the FWS_CHK loop (polling FDC_SENSE_INT and testing bit 5) so both
loops break after N tries and return a defined error path instead of spinning
forever.
---
Nitpick comments:
In `@source/kernel/bank6/format.mac`:
- Around line 338-340: The symbol ?FMT_PRINT_AND_CHOOSE is exported twice (via
the `public ?FMT_PRINT_AND_CHOOSE` directive and the `?FMT_PRINT_AND_CHOOSE::`
label); remove the redundant export by changing the label to a normal local
label (remove the `::` suffix) and keep the existing `public
?FMT_PRINT_AND_CHOOSE` declaration so the symbol is exported consistently with
other entry points like ?BUILD_FMT_CHOICES and ?F_FORMAT.
- Around line 86-92: The DRVTBL walker loop at label fformat_drvtbl can run
unbounded if DRVTBL entries are inconsistent; add a 4-iteration safety counter
(matching the 4 DRVTBL slots) before the loop (e.g., load a register with 4) and
decrement it each iteration, branching to a new failure path (e.g.,
fformat_drvtbl_not_found) when it reaches zero; update the loop around the
existing sub (hl), jr c,fformat_found_drv and inc hl/inc hl sequence so the
counter is checked after each iteration to prevent reading past DRVTBL.
In `@source/kernel/const.inc`:
- Around line 244-260: The UFM_FD mask line uses uppercase "EQU" while the
surrounding definitions use lowercase "equ"; change the directive for UFM_FD
(the line defining UFM_FD / symbol UFM_FD or UF_FD context) to use lowercase
"equ" to match the file's style so casing is consistent with the other mask
definitions (e.g., UFM_DB, UFM_RM).
In `@source/kernel/drivers/TurboRFDD/chgbnk.mac`:
- Around line 27-30: The comment describing Sunrise IDE's bit-swapping for bank
bits is stale and misleading for this file: the actual CHGBNK implementation
uses a plain ASCII16 mapper (the code writes A to (6000h)), so remove or replace
the Sunrise-specific bit-swapping notes with a short, accurate comment stating
that CHGBNK performs a direct write to 0x6000 for ASCII16 bank switching
(reference symbol: CHGBNK and the write to (6000h)). Ensure the new comment
clearly identifies the mapper behavior and does not mention
Sunrise/bit-reversal.
In `@source/kernel/drivers/TurboRFDD/driver.mac`:
- Around line 331-369: The stub routines OEMSTAT, BASDEV, EXTBIO, and
DIRECT_0–DIRECT_4 contain redundant trailing `ret` instructions (e.g., `scf;
ret; ret`) which are dead bytes; either remove the extra `ret` lines so each
stub contains only the minimal return sequence, or if the extra `ret`s are
intended as padding/alignment, add a brief comment above each label (e.g.,
"padding for alignment/expansion") clarifying the intent; update the labels
OEMSTAT, BASDEV, EXTBIO, DIRECT_0, DIRECT_1, DIRECT_2, DIRECT_3, and DIRECT_4
accordingly.
- Around line 1973-2014: CALL_XFER must validate the buffer is not in page 1
before patching/switching pages: after DE is set to the buffer (the ex de,hl
that yields "DE = buffer") add a runtime check on DE's high byte (D) to detect
addresses in 4000h–7FFFh (page 1) and branch to a new handler (e.g.,
XFER_BAD_BUFFER) that returns an error or aborts the transfer instead of
continuing; reference the CALL_XFER label, the DE/HL registers, and the
page-switching sequence that uses WK_XFER/WK_FLAGS/XFER_PATCH_OFF so the guard
runs before the push/pop/page-switching code and prevents the RAM routine jump
(jp (iy)) when the buffer is in page 1.
In `@source/kernel/Makefile`:
- Line 67: Declare the Make target all as phony and fix the incorrect lowercase
phony declaration: add a .PHONY line that includes the all target (e.g.,
".PHONY: all" alongside other phony targets) and replace any ".phony:"
occurrences with the correct ".PHONY:" to ensure GNU Make treats these targets
as phony.
RR with the carry set could have caused an extra bogus drive letter to be shown as formattable in the DOS 1 version of CALL FORMAT.
- DSK_CHOICE_BUF moved to bank 6 - Buffer size for choice strings is now passed in DE, not D
… handling F_FORMAT (DOS 1) and DSK_CHOICE_BUF (DOS 2) now use SECBUF as an intermediate buffer when querying Nextor drivers for format choices, then copy to the user buffer with page 2 segment remapping. This fixes cross-page boundary issues when drivers write directly (B=255) and allows user buffers in any memory page. Buffer size is capped at 512 bytes (returns .IFORM if exceeded).
Konamiman
force-pushed
the
better-support-for-floppies
branch
from
a5d743d to
2276037
Compare
Konamiman
force-pushed
the
better-support-for-floppies
branch
from
2276037 to
232027e
Compare
…ntime ghost creation - Add missing jump after SETUP_GHOST_DOS1 in init.mac that caused fall-through into MK_EMU_TBL, corrupting KERNEX_DOS1 with a page 2 address - Add ghost drive detection to GDLI_DOS1 using UD1_FIRST_ABSOLUTE_SECTOR state bits, so CALL DRVINFO shows ghost status in DOS 1 mode - Fix MAYBE_CREATE_GHOST_DOS1: use arithmetic to check for next entry instead of comparing drive numbers with entry count (which fails when legacy drivers are present), and only check ghost state bits on FDD entries since non-FDD entries have actual sector numbers there - Remove GHO_PAIR dependency from DOS 1 ghost cleanup and creation, using KERNEX_DOS1 entry data directly instead - Have SETUP_GHOST_DOS1 increment DRVTBL/$NUMDRV directly instead of deferring to init.mac via GHO_PAIR - Add page 2 segment switch around UNDO_GHOST_DRVTBL call for correct GHO_PAIR access
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
source/kernel/bank6/format.mac (1)
86-96:⚠️ Potential issue | 🔴 CriticalReject zero-length buffers before entering copy paths.
A zero buffer size is currently accepted, then copy loops decrement
BC/DEand underflow, which can write out of bounds before termination handling kicks in.💡 Proposed fix
@@ ld a,c cp 80h jr nz,fformat_size_ok + ld a,d + or e + jp z,fformat_pop_iform push hl ld hl,512 or a sbc hl,de ;Carry if DE > 512 @@ ?DSK_CHOICE_BUF: ;--- Check buffer size limit (max 512) push hl + ld a,d + or e + jr z,dcb_bad_size ld hl,512 or a sbc hl,de ;Carry if DE > 512 pop hl jr nc,dcb_size_ok +dcb_bad_size: ld a,.IFORM## or a ;NZ retAlso applies to: 231-248, 304-329, 738-749, 903-920
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/bank6/format.mac` around lines 86 - 96, The code currently allows a zero-length buffer (C=0) which then enters copy paths that decrement BC/DE and can underflow; modify the pre-copy size checks (e.g., the C/512 check sequence around label fformat_size_ok and the entry that jumps to fformat_pop_iform) to explicitly reject or branch out when C==0 (and similarly when BC or DE are zero in the other copy-entry sequences), so that zero-length buffers are detected and handled before any decrement-based copy loops run; apply the same guard logic to the other copy entry sites noted (around source ranges 231-248, 304-329, 738-749, 903-920) to prevent underflow.
🧹 Nitpick comments (2)
source/kernel/drivers/StandaloneASCII8/driver.mac (1)
541-551: Minor documentation style inconsistency.The output documentation format differs from the other queries. Queries 5 and 6 use
A = QUERY_OK:format while this one omits theA =prefix.📝 Suggested fix for consistency
; Device query 7: Stop the floppy disk drive motor ; ; Input: - -; Output: QUERY_OK: ok, motor has been stopped +; Output: A = QUERY_OK: ok, motor has been stopped ; QUERY_INVALID_DEVICE: device does not exist ; QUERY_NOT_IMPLEMENTED: the device is not a floppy disk ; or stopping the drive motor is not supported🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/drivers/StandaloneASCII8/driver.mac` around lines 541 - 551, The documentation comment for the DO_DEVQ_STOP_MOTOR query is missing the "A = " prefix used elsewhere; update the comment block above DO_DEVQ_STOP_MOTOR so each output line uses the same format (e.g. "A = QUERY_OK: ok, motor has been stopped", "A = QUERY_INVALID_DEVICE: device does not exist", "A = QUERY_NOT_IMPLEMENTED: the device is not a floppy disk or stopping the drive motor is not supported") to match Queries 5 and 6.source/kernel/Makefile (1)
67-67:alltarget should be declared.PHONY.As flagged by static analysis, the
alltarget should be added to a.PHONYdeclaration. Additionally, line 71 uses lowercase.phonywhich may not be recognized by allmakeimplementations.Suggested fix
-all: base ide ide-masteronly ide-blue ide-masteronly-emu ascii8 ascii16 mfrsd flashjacks ocm dsk-tr +.PHONY: all +all: base ide ide-masteronly ide-blue ide-masteronly-emu ascii8 ascii16 mfrsd flashjacks ocm dsk-trOr consolidate with the existing declaration at line 71:
-.phony: prerequisites +.PHONY: all prerequisites clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/Makefile` at line 67, Add the "all" target to the phony declarations and correct the lowercase `.phony` to the standard `.PHONY`; specifically, update the existing phony declaration (or create one) to include the target name "all" and ensure the special target uses uppercase `.PHONY` so all make implementations treat "all" as phony rather than a real file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ai/PLAN.md:
- Line 50: Fix the typo in the sentence that currently reads "This implies that
a boot time the device will get exactly one drive assigned (but later w'll
discuss about ghost drives)." by replacing "w'll" with "we'll" so the phrase
becomes "... (but later we'll discuss about ghost drives)." Ensure you update
the exact snippet containing "w'll" in the document.
- Line 118: Fix the typo in the PLAN doc: change "unknwon" to "unknown" in the
sentence that lists QUERY_NOT_IMPLEMENTED (the clause mentioning floppy disk
drive/formatting/unknwon choice) so it reads "...or an unknown choice is
passed".
- Line 29: Fix the typo in the PLAN document: replace "dinamically" with
"dynamically" in the sentence describing "bit 1" (the line containing "bit 1: 1
if the device is read only. A device that can dinamically") so it reads "A
device that can dynamically". Ensure only the misspelled word is corrected and
punctuation remains unchanged.
In `@source/kernel/bank0/init.mac`:
- Around line 308-309: The key inversion constant KEYS_INV_1 is currently
flipping bit 4 (10h) which inverts SHIFT, not CTRL; update the value used by
KEYS_INV_1 so it toggles bit 5 (20h) instead—i.e., replace the 10h mask with 20h
(or otherwise ensure the db emitted for KEYS_INV_1 sets bit 5) so the symbol
KEYS_INV_1 actually inverts CTRL as intended.
In `@source/kernel/drivers/TurboRFDD/driver.mac`:
- Around line 1737-1739: FDC_WAIT_READY currently ignores the return carry from
FDC_WRITE_CMD and proceeds to FDC_READ_RESULT, risking reading stale data on
timeout; modify FDC_WAIT_READY to test the carry flag after the CALL
FDC_WRITE_CMD (like the pattern used in FDC_SENSE_INT) and branch to the outer
timeout/cleanup path if carry is set (i.e., treat a set carry as a write timeout
and avoid calling FDC_READ_RESULT), ensuring consistent timeout handling.
---
Duplicate comments:
In `@source/kernel/bank6/format.mac`:
- Around line 86-96: The code currently allows a zero-length buffer (C=0) which
then enters copy paths that decrement BC/DE and can underflow; modify the
pre-copy size checks (e.g., the C/512 check sequence around label
fformat_size_ok and the entry that jumps to fformat_pop_iform) to explicitly
reject or branch out when C==0 (and similarly when BC or DE are zero in the
other copy-entry sequences), so that zero-length buffers are detected and
handled before any decrement-based copy loops run; apply the same guard logic to
the other copy entry sites noted (around source ranges 231-248, 304-329,
738-749, 903-920) to prevent underflow.
---
Nitpick comments:
In `@source/kernel/drivers/StandaloneASCII8/driver.mac`:
- Around line 541-551: The documentation comment for the DO_DEVQ_STOP_MOTOR
query is missing the "A = " prefix used elsewhere; update the comment block
above DO_DEVQ_STOP_MOTOR so each output line uses the same format (e.g. "A =
QUERY_OK: ok, motor has been stopped", "A = QUERY_INVALID_DEVICE: device does
not exist", "A = QUERY_NOT_IMPLEMENTED: the device is not a floppy disk or
stopping the drive motor is not supported") to match Queries 5 and 6.
In `@source/kernel/Makefile`:
- Line 67: Add the "all" target to the phony declarations and correct the
lowercase `.phony` to the standard `.PHONY`; specifically, update the existing
phony declaration (or create one) to include the target name "all" and ensure
the special target uses uppercase `.PHONY` so all make implementations treat
"all" as phony rather than a real file.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.ai/PLAN.mdsource/kernel/Makefilesource/kernel/bank0/init.macsource/kernel/bank2/misc.macsource/kernel/bank2/val.macsource/kernel/bank3/dos1ker.macsource/kernel/bank4/partit.macsource/kernel/bank6/bootmsg.macsource/kernel/bank6/format.macsource/kernel/bank6/ghost.macsource/kernel/bank6/jtable.incsource/kernel/drivers/StandaloneASCII8/driver.macsource/kernel/drivers/TurboRFDD/driver.mac
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ai/PLAN.md:
- Around line 26-35: The fenced code blocks in .ai/PLAN.md that start with the
assembly comment line "; +7 (1): Flags:" and the three blocks beginning with
"3.83 FORMAT A DISK (67H)" and the two "Input: B = Device number" blocks are
missing language identifiers; update those fences to include proper languages
(e.g., ```asm for the assembly/comment block and ```text for the plain text/CLI
examples) so markdownlint MD040 is satisfied and readability improves.
- Line 16: Normalize compound modifiers by adding hyphens for consistency:
replace occurrences of phrases like "single sided/double sided" with
"single-sided/double-sided", "three step" with "three-step", "MSX-DOS 1
compatible" with "MSX-DOS 1-compatible", and "RAM based" with "RAM-based"
throughout the .ai/PLAN.md content (including the noted occurrences around the
quoted phrase and lines 55, 59, 88, 152, 154); ensure similar compound modifiers
follow the same hyphenation pattern and run a quick pass with LanguageTool or a
regex search to catch remaining instances.
- Line 156: Update the sentence about register C to state that register C is
repurposed to carry the media id for floppy accesses (passed to the READ_WRITE
driver routine) rather than being "currently unused"; reference that this
mirrors Nextor 2's use of C for LUN but clarify the change is specific to floppy
devices and not a general LUN assignment. Ensure the wording mentions "register
C", "READ_WRITE driver routine", "media id", and "floppy disk" so readers won't
interpret it as an unused register.
In `@source/kernel/drivers/TurboRFDD/driver.mac`:
- Around line 1570-1574: The loops labeled FWC_LP and FRR_LP busy-wait
indefinitely on READ_MSR which can hard-lock the system; add bounded polling by
introducing a timeout counter (e.g., a local register or label-based counter) in
both FWC_LP and FRR_LP that decrements each iteration and branches to a common
error/timeout handler if it reaches zero, and ensure the handler performs
appropriate cleanup/abort (set error/status, return or jump to existing error
path) so stalled controllers cannot hang the kernel; reference the FWC_LP and
FRR_LP labels and the READ_MSR wait sequence when inserting the counter and
branch to the new timeout handler.
- Around line 1009-1013: DI_WRITE_SECTORS currently returns based solely on the
carry flag after calling READ_WRITE, but READ_WRITE signals success/failure in
register A; update DI_WRITE_SECTORS to check A (the error code) instead of only
testing carry so failed writes don't get treated as successes. Locate the
sequence calling READ_WRITE (the call READ_WRITE / pop bc / pop hl block) and
replace the unconditional "ret c" behavior with a check of A (e.g., test for
A==0 for success and return/branch on A!=0), preserving the pops and existing
stack cleanup; ensure any branches that previously relied on carry now use A to
decide error handling so format initialization aborts on nonzero A.
69d5ae1 to
43938cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.ai/PLAN.md (3)
26-35:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks (MD040).
These fenced blocks are still missing language tags (
asm/text), so markdownlint will keep warning and readability stays lower.Also applies to: 63-79, 97-109, 113-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ai/PLAN.md around lines 26 - 35, The fenced code blocks in the provided markdown (the triple-backtick blocks containing the semicolon-prefixed assembly-like comments) are missing language identifiers; update each fenced block (including the one shown and the others noted) to add an appropriate language tag such as asm or text (e.g., change ``` to ```asm or ```text) so markdownlint MD040 warnings are resolved and readability improves.
156-156:⚠️ Potential issue | 🟠 MajorClarify register C wording to avoid Nextor 2 compatibility confusion.
“currently unused” is misleading here; this is a floppy-specific repurposing relative to Nextor 2 LUN usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ai/PLAN.md at line 156, Update the sentence about passing the media id to the READ_WRITE driver routine in register C to remove "currently unused" and explicitly state that register C is repurposed for floppy devices (i.e., used to pass a floppy media id) and that this is a floppy-specific behaviour relative to Nextor 2 LUN usage; mention the READ_WRITE driver routine, "register C", "media id", and "Nextor 2 LUN" to make the compatibility distinction clear.
16-16:⚠️ Potential issue | 🟡 MinorNormalize compound modifiers with hyphens consistently.
Terms like “single-sided/double-sided”, “three-step”, “MSX-DOS 1-compatible”, and “RAM-based” are still unhyphenated in several places.
Also applies to: 55-55, 59-59, 88-88, 152-152, 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ai/PLAN.md at line 16, Normalize compound modifiers in .ai/PLAN.md by adding hyphens consistently for adjectival compounds: change instances like "single sided/double sided disks" to "single-sided/double-sided disks", "three step" to "three-step", "MSX DOS 1 compatible" (or "MSX-DOS 1 compatible") to "MSX-DOS 1-compatible", and "RAM based" to "RAM-based"; apply the same normalization at the other reported occurrences (lines referenced 55, 59, 88, 152, 154) so all compound modifiers are hyphenated when used as descriptors.
🧹 Nitpick comments (1)
source/kernel/drivers/TurboRFDD/driver.mac (1)
332-370: Stubs contain unreachableretinstructions.Each stub routine has one or two extra
retinstructions after the firstret(e.g., lines 335, 340, 344-345). These are unreachable dead code. If this is intentional padding for entry point alignment, a comment would clarify the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/kernel/drivers/TurboRFDD/driver.mac` around lines 332 - 370, The listed stub routines (OEMSTAT, BASDEV, EXT BIO, DIRECT_0..DIRECT_4) contain extra unreachable `ret` instructions after the first `ret`; remove the duplicate `ret` instructions in each label so each stub has a single return, or if the extra `ret`s are intentional padding for alignment, add a short comment at each label (e.g., "padding for entry alignment") clarifying intent; update the labels OEMSTAT, BASDEV, EXT BIO, DIRECT_0, DIRECT_1, DIRECT_2, DIRECT_3, and DIRECT_4 accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/kernel/drivers/TurboRFDD/driver.mac`:
- Around line 2068-2069: The READ_WRITE entry point allows HL as the buffer
address but lacks validation that HL is in page 2 or 3 (0x8000–0xFFFF); add a
runtime check at the start of READ_WRITE that tests HL’s high byte (or the
address) and branches to a safe error/return path (or sets an error flag/FAIL
condition) if HL < 0x80, or alternatively add a clear debug-only assertion
comment and trap to aid development; reference the READ_WRITE entry and the HL
buffer argument when adding the check so callers cannot pass buffers in page
0/1.
---
Duplicate comments:
In @.ai/PLAN.md:
- Around line 26-35: The fenced code blocks in the provided markdown (the
triple-backtick blocks containing the semicolon-prefixed assembly-like comments)
are missing language identifiers; update each fenced block (including the one
shown and the others noted) to add an appropriate language tag such as asm or
text (e.g., change ``` to ```asm or ```text) so markdownlint MD040 warnings are
resolved and readability improves.
- Line 156: Update the sentence about passing the media id to the READ_WRITE
driver routine in register C to remove "currently unused" and explicitly state
that register C is repurposed for floppy devices (i.e., used to pass a floppy
media id) and that this is a floppy-specific behaviour relative to Nextor 2 LUN
usage; mention the READ_WRITE driver routine, "register C", "media id", and
"Nextor 2 LUN" to make the compatibility distinction clear.
- Line 16: Normalize compound modifiers in .ai/PLAN.md by adding hyphens
consistently for adjectival compounds: change instances like "single
sided/double sided disks" to "single-sided/double-sided disks", "three step" to
"three-step", "MSX DOS 1 compatible" (or "MSX-DOS 1 compatible") to "MSX-DOS
1-compatible", and "RAM based" to "RAM-based"; apply the same normalization at
the other reported occurrences (lines referenced 55, 59, 88, 152, 154) so all
compound modifiers are hyphenated when used as descriptors.
---
Nitpick comments:
In `@source/kernel/drivers/TurboRFDD/driver.mac`:
- Around line 332-370: The listed stub routines (OEMSTAT, BASDEV, EXT BIO,
DIRECT_0..DIRECT_4) contain extra unreachable `ret` instructions after the first
`ret`; remove the duplicate `ret` instructions in each label so each stub has a
single return, or if the extra `ret`s are intentional padding for alignment, add
a short comment at each label (e.g., "padding for entry alignment") clarifying
intent; update the labels OEMSTAT, BASDEV, EXT BIO, DIRECT_0, DIRECT_1,
DIRECT_2, DIRECT_3, and DIRECT_4 accordingly.
43938cd to
d2915ec
Compare
Konamiman
added a commit
that referenced
this pull request
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add comprehensive floppy disk drive support for Nextor drivers. A device is considered to be a floppy disk if its "floppy disk" flag is set in the information returned by the "Get device parameters" device query.
Also and a new driver for the MSX Turbo-R floppy disk hardware is introduced.
No partitions
When a drive is assigned to a floppy disk Nextor will not search for partitions: it will always assign the drive to the absolute sector zero of the device.
Also FDISK will list floppy disk devices, but will not allow to manipulate them.
Media ID for single side/double site detection
Nextor will now pass the media id byte of the inserted disk to the
READ_WRITEroutine of the driver when accessing a floppy disk. This allows the driver to perform the necessary adjustments for single sided disks or in other similar cases of special geometries.The media id byte is passed in register C, note that in Nextor 2 this register was used to pass the LUN number.
Support for ghost drives
"Ghost drives" is a mechanism that allows to exchange information between different floppy disks when having one single disk drive. The drive gets two drive letters, and when accessing the second drive, the system prompts "Insert disk for drive X: and press any key" to give the user an opportunity to swap the disk.
In MSX-DOS this mechanism is handled exclusively by the disk driver (it announces two drives, and it explicitly calls a kernel-provided routine for the disk change prompt). In Nextor however, it's the kernel who handles everything, and the driver only needs to provide the standard read/write sectors routine that's also used for non-floppy devices (and appropriately set the "floopy disk" flag for the device).
Only one ghost drive can be assigned as ghost drive at the same time, and the ghost drive letter for a floppy disk drive is always the next one.
Assigning ghost drives
At boot time, when Nextor assigns a drive to a device it will also assign the following drive as its ghost drive if all the following is true:
Later on, when manually mapping a drive letter to a device, the following drive letter will be assigned as its ghost following a similar logic:
Unmapping a ghost drive letter has no effect in its "master" drive. Unmapping the "master" drive will automatically unmap the ghost drive too.
Changes in _GDLI and MAPDRV
The
_GDLIfunction call has been extended to provide information about ghost drives:Note that currently a ghost drive will always be ghosting the drive immediately preceding it, but the main drive number is returned explicitly anyway in case this restriction is lifted in a future version of Nextor.
The
CALL MAPDRVcommand and theMAPDRV.COMtool will now properly show ghost drives as such.Handling disk changes
The kernel will internally track which drive has a ghost assigned and which one of the pair has last been accessed, as to show the "Insert disk..." prompt as needed. This is completely transparent for drivers.
Formatting disks
Support for formatting floppy disks controlled by Nextor kernels is added as well.
New "get choices" driver query
A new driver query is added to get the formatting choices for a disk:
The most common formatting choices for floppy disks are either "Single side / double side, double density" or "Single side / double side, double density / double side, high density", for 3.5" and 5.25" disks. If the choices supported by the driver are one of those (or if there's only one choice available), the driver can return B=0, 1 or 2 and then the kernel will show an appropriate built-in message (which doesn't include any disk capacity numbers, that's on purpose). Otherwise, the driver can copy a custom choice string to the buffer address provided in HL and return B=255, then the kernel will use this string.
New "format a disk" driver query
The other new query is the one to actually format the disk:
As expected, choice numbers correspond to the format choices returned by the "get choices" query. After the formatting is completed the driver must initialize the disk with a MSX-DOS 1 compatible boot sector, empty FAT, and empty root directory.
Changes in the _FORMAT function call
The
_FORMATfunction calls changes as follows:Note that:
Changes in CALL FORMAT and the FORMAT command
CALL FORMATworks as expected: it lists all the MSX-DOS drives and all the drives assigned to Nextor floppy disk devices, shows the choice string as appropriate, delegates the formatting process to the driver, and (except in MSX-DOS 1 mode) adjusts the boot sector to the new MSX-DOS 2 format.The
FORMATcommand that's built-in in COMMAND2.COM, however, still uses the choice number 0 to get the choice string, and therefore it will work only for MSX-DOS drives. To format disks controlled by Nextor drivers, eitherCALL FORMATfrom BASIC or a custom tool must be used.AI usage
The development of this feature has been assisted by an AI agent (Claude). For educational purposes, a transcript of the initial session with the agent is included in this pull request: