Skip to content
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

Experimental audio playback support from BIN/CUE files. #207

Merged
merged 16 commits into from May 14, 2023

Conversation

saybur
Copy link
Contributor

@saybur saybur commented May 12, 2023

First attempt at getting audio playback working with the new BIN/CUE support. This has been tested to play audio successfully using 'cdplay' on a modern Linux host. There are bugs and this is not ready for merging, but hopefully the PR can give an idea of potential changes for CD-DA. I'll keep developing this toward something more polished. Known issues:

  • Annex C rules for commands to halt audio playback are not supported yet. This prevents playback from being stopped until the initial play command naturally terminates (pausing does work).
  • Position tracking is limited to just audio playback, probably needs READ integration.
  • Additional mode page support may be needed, I only added a boilerplate 0Eh. Volume control in particular might be pretty useful, but AFAIK there isn't much MODE SELECT support in the firmware?
  • Testing has been pretty basic and needs to be expanded, particularly on period-correct systems.
  • Testing of non-audio firmware behavior with audio commands needs to be done.

Copy link
Collaborator

@PetteriAimonen PetteriAimonen left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left a few small notes, but nothing critical.
Let us know when you consider it ready for merging.

src/ImageBackingStore.cpp Show resolved Hide resolved
lib/SCSI2SD/src/firmware/mode.c Show resolved Hide resolved
* The first two are for a live condition and will be returned repeatedly. The
* following two reflect a historical condition and are only returned once.
*/
enum audio_status_code {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would these parts be common to all platforms if another platform gets audio support in future?
If yes, it could be better to make src/ZuluSCSI_audio.h for the common parts and interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll resolve that.

saybur added 12 commits May 13, 2023 06:42
Also adds image position() call, for audio checking and removal of separate mechanism tracking system.
RP2040 scsiPhy.cpp drops to SCSI1 compatibility when IDENTIFY messages are not received, preventing this page from being sent to some hosts (68k Macs) otherwise expecting it.
I'm a novice at C++ and I'm concerned this is not good practice, I'm open to a different approach: goal here is to share underlying resources including position tracking.
Fixing my error from prior commit and properly handling current position playback request, hopefully.
@saybur
Copy link
Contributor Author

saybur commented May 14, 2023

Should now be minimally functional across platforms. Testing has shown lots of SCSI command variability across audio playback software, so if merged this probably needs a public 'still experimental' disclaimer for any users. Known bugs include:

  • Playback progress displays are wrong on at least the stock Mac and Win9x players.
  • SCAN is not implemented, which could lead to incorrect behavior when fast-forwarding/rewinding. Software observed so far doesn't actually use this command, it instead just issues a new play command.
  • The code has some guesses about how to report positioning. I may not be understanding the spec here. It is starting and stopping where requested so far.

If this seems like it needs refactoring, more time in the oven, etc, let me know.

@saybur saybur marked this pull request as ready for review May 14, 2023 16:23
@PetteriAimonen
Copy link
Collaborator

Looks good to merge in my opinion.

I agree that it should be considered experimental feature, but I guess the related hardware is also still experimental.

@aperezbios aperezbios merged commit 20ed645 into ZuluSCSI:main May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants