-
-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix daynaport emulation regression (#1306) #1318
Conversation
I'd highly recommend testing this on a SE or older machine before merging. The "obsolete" work around was only needed for slower machines. |
@akuker Do you know any user who could run this test? |
@uweseimet I have a Mac SE as well. I'll see if this will work there later. |
This reverts commit a46880f.
@benjamink Excellent! I will reset this PR to a draft for now, so that we can wait for your feedback. |
This reverts commit c46d3b7.
Because the delay handling is not critical I am going to update the branch to only contain the actual fix. Regarding the delay I am going to create a separate ticket. If until the next release there is no use case (slow Mac) that requires the delay it should be removed. The comment in the sources does not mention slow Macs but sounds to me as if the Daynaport driver needs this delay. This is not necessarily the case, as the testing by @benjamink has shown. The work-around for 37 bytes (INQUIRY), on the other hand, is definitely needed, the latest tests and logs prove that. I will update the respective comment for one of the next PRs, so that it reflects the details on why this other work-around is required. @rdmark With this approach I don't see any reason not to merge. Only a single line has changed. Essentially, only one character ;-). |
Kudos, SonarCloud Quality Gate passed! |
I think I may need to come up with a new disk image for my Mac SE to work at all. I've been trying to boot my System 7.5.3 image that I've been using with my Performa 475 but it just freezes on the boot screen. I can boot up a System 6.0.8 image just fine but it doesn't have Daynaport drivers or anything like that installed yet. I'll work on this & see what I can come up with but it won't be right away. Unless someone else has a pre-configured System 7.0.1 or something I could use??? |
@benjamink Just take your time. The respective test can be done as part of a new ticket, which I will create (also see my comment above). As soon as I have set up the ticket (tomorrow) I will point it out to you, and any conversation can then happen as part of this new ticket. |
@uweseimet SGTM. How about we aim for 11/11 (Saturday) for tagging & prepping the next release? I expect to have the time to spend on the image generation etc. on that day. |
@uweseimet I built |
@benjamink In this case we will keep this delay, of course, but the current code location setting up this delay is not ideal, and there are other delays that also need verification. Would you be willing to help with improving that? There is already an existing ticket (#1287) for addressing this. The goal is verify which delay is actually needed, whether there is a better way to address the problem, and to add a comment to the sources explaining the reason for any delay that is not covered by the SCSI specification in detail. As we can see in #1098 some of these delays can actually cause compatibility issues. |
Yes, I can test. I moved the SE upstairs in anticipation of running more tests already 🙂 |
@benjamink :-) I really appreciate your help. Let's continue this conversation in #1287, which is the respective ticket. |
Successfully tested by @benjamink.