Skip to content

Conversation

@LowLevelMahn
Copy link
Contributor

Description of Changes

change the encode/decoding of the PSP Commandline

Rationale behind Changes

Behavior was different to native DOS or dosbox, did not work properbly

Suggested Testing Steps

there is a DosProgramSegmentPrefixCmdTest to test encoding/decoding with some cases

@LowLevelMahn LowLevelMahn force-pushed the llm/commandline_fix branch 5 times, most recently from dd8dcab to fbc900a Compare December 5, 2025 16:51
@maximilien-noal maximilien-noal self-requested a review December 5, 2025 18:55
@maximilien-noal maximilien-noal added DOS Related to DOS bugfix fixes a bug compatibility Emulator compatibility with DOS apps labels Dec 5, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the PSP (Program Segment Prefix) command line encoding/decoding to match native DOS and DOSBox behavior. The implementation addresses incorrect handling of spaces, length calculation, and termination characters in the DOS command tail structure.

Key changes:

  • Introduces PrepareCommandlineString method to normalize command line arguments with proper DOS formatting (leading space, trailing whitespace trimming, length limits)
  • Refactors DosCommandTail.Command property with explicit ASCII encoding/decoding instead of relying on zero-terminated string helpers
  • Adds comprehensive test coverage with multiple command line scenarios including empty strings, whitespace handling, and special characters

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File Description
tests/Spice86.Tests/Dos/DosProgramSegmentPrefixCmdTest.cs New test file with 6 test cases validating command line encoding against DOS byte format expectations
src/Spice86.Core/Emulator/OperatingSystem/Structures/DosCommandTail.cs Adds PrepareCommandlineString and CheckParameterString methods; refactors Command getter/setter with explicit byte-level encoding; updates constants
src/Spice86.Core/Emulator/OperatingSystem/DosProcessManager.cs Removes obsolete ArgumentsToDosBytes method; simplifies PSP command tail setup to use new PrepareCommandlineString

Copy link
Member

@maximilien-noal maximilien-noal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a few suggestions. :)

Thank you for this @LowLevelMahn !

Really appreciate it.

It's much better than the old code.


// there needs to be a blank as first char in parameter string, if there isn't already
if (ag[0] != ' ') {
ag = ' ' + ag;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be ag = $" {ag}";


public const int MaxCharacterLength = 128;
public const int MaxSize = 128;
public const int MaxCharacterLength = MaxSize - 2; // length-byte + 126 chars max + \r
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be visible for users of the Nuget package, this comment could be a ///

above the const.

for (int i = 0; i < length; i++) {
buffer[i] = UInt8[(uint)(1 + i)];
}
return Encoding.ASCII.GetString(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldnt this work?

        get => Encoding.ASCII.GetString(GetUInt8Array(1, Length).ToArray());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix fixes a bug compatibility Emulator compatibility with DOS apps DOS Related to DOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants