libusb driver

[elmattic]

No description provided.

[joshstoik1]

Thanks @elmattic !

I pushed a quick edit to make the CI happy, but otherwise it's looking good at a glance. I'll give the fork a proper test on Linux later to see how it's doing.

One small thing: can you move the cdtext reference files to a subfolder, skel/cdtext or some such?

[joshstoik1]

The libusb fork compiled okay on Linux, but it isn't playing nice with my PIONEER BD-RW BDR-X13U drive; a Internal("Pipe error") error triggers here during the initial setup.

I remember your first PR had trouble identifying my drive and offset, so I suspect that's probably still the case, there's just more error-handling now. Haha.

Not sure how helpful it is, but the state before that point looks like this:

cbw: CommandBlockWrapper {
    signature: 1128420181,
    tag: 268435457,
    data_transfer_length: 804,
    flags: 128,
    lun: 0,
    cb_length: 10,
    cdb: [67, 0, 0, 0, 0, 0, 1, 3, 36, 0, 0, 0, 0, 0, 0, 0]
}
transferred:  84
buf[..84]: [0, 82, 1, 9, 0, 16, 1, 0, 0, 0, 0, 0, 0, 16, 2, 0, 0, 0, 88, 117, 0, 16, 3, 0, 0, 0, 177, 110, 0, 16, 4, 0, 0, 1, 24, 215, 0, 16, 5, 0, 0, 1, 127, 94, 0, 16, 6, 0, 0, 1, 190, 76, 0, 16, 7, 0, 0, 2, 8, 32, 0, 16, 8, 0, 0, 2, 107, 158, 0, 16, 9, 0, 0, 2, 193, 7, 0, 16, 170, 0, 0, 3, 11, 128]
self.device_handle: DeviceHandle {
    device: Bus 004 Device 004: ID 08e4:017a,
    handle: Some(0x5624297c07e0),
    interfaces: ClaimedInterfaces { inner: [1, 0] }
}

Is there anything you'd like me to try to help narrow the issue down, or should I just sit back and wait for the PR to bake a bit more?

[elmattic]

Great success! I love those hardware errors.

So far, I’ve only tested it on my Verbatim Ultra HD 4K Blu-ray Writer (HL-DT-ST BD-RE BU40N). I haven’t had many issues on either macOS or Fedora (that doesn’t mean it’s rock solid, though, as you can see).

The first thing I’d check is whether the MMC command for READ_TOC is being crafted properly.

The logic for handling stalls in LibusbInstance::submit could probably be rethought as well. Etc.

I’ll keep you posted when I know more. Can you just confirm the error is happening during check_disc_mode__()?

[elmattic]

Btw, is the error fully reproducible?

[elmattic]

As you can see in the MMC payload for check_disc_mode__(), I always create the command for the worst-case scenario (i.e. 99 tracks, according to the Red Book specification).

Could you simply hardcode the number of tracks on your audio CD? How many have you, 9? We’re asking for 804 bytes, but your drive only has 84 bytes of TOC data.

A standards-compliant device should simply return 84 bytes and then the CSW. But optical-drive firmware can be surprisingly quirky. Haha.

[elmattic]

I remember your first PR had trouble identifying my drive and offset, so I suspect that's probably still the case

How can we make sure the drive is properly detected and offset correctly set?

[elmattic]

A standards-compliant device should simply return 84 bytes and then the CSW. But optical-drive firmware can be surprisingly quirky. Haha.

I don't know enough about the BOT protocol yet, but I suspect that some firmware/controllers may also return data in a short packet. In that case, we will need to check the CSW for the residual data bytes and, if the residual is > 0, call read_bulk again.

[joshstoik1]

Great success! I love those hardware errors.

Optical drives are such quirky little shits. Haha. A bit lot of whack-a-mole is to be expected for sure!

Can you just confirm the error is happening during check_disc_mode__()?

Yeah, failure is pretty immediate when the ALLOC_LEN multiplier is larger than the number of tracks. Here are two example discs:

# 9+96+590B+B204+1196D+17FF4+1BEE2+208B6+26C34+2C19D+30C16
check_disc_mode__
  submit
    transferred: 84
    Err3 Pipe error # csw_raw read

# 4+B6+76B6+10635+1575C+1B64A
check_disc_mode__
  submit
    transferred: 44
    Err3 Pipe error

Could you simply hardcode the number of tracks on your audio CD?

Manually changing the 99 to a value less than or equal to the number of tracks, things progress a little farther:

# 9+96+590B+B204+1196D+17FF4+1BEE2+208B6+26C34+2C19D+30C16
check_disc_mode__
  submit
    transferred: 84
check_c2__
  submit
    transferred: 16
read_cdtext
  submit
    transferred: 580 # This disc has CDText
    Err3 Pipe error
get_toc_header
  submit
    Err0 Operation timed out # cbw_bytes write

# 4+B6+76B6+10635+1575C+1B64A
check_disc_mode__
  submit
    transferred: 44
check_c2__
  submit
    transferred: 16
read_cdtext
  submit
    transferred: 4 # This disc does not.
    Err3 Pipe error
get_toc_header
  submit
    Err0 Operation timed out

How can we make sure the drive is properly detected and offset correctly set?

The DriveVendorModel automatically applies (known) read offsets, so the trick is just to make sure drive_vendor_model__ correctly picks up the vendor/model details.

Rip Rip isn't making it to this stage so I don't know if detection would still be an issue for me or not, but looking at the code I suspect it would be for at least some drives because std::str::from_utf8(vendor_raw).ok()?.trim() doesn't account for null bytes.

For the libcdio driver, I had to stringify the values like this instead:

// Vendor might be empty.
let vendor =
	if vendor_u8[0] == 0 { "" }
	else {
		CStr::from_bytes_until_nul(vendor_u8.as_slice())
		.ok()
		.and_then(|v| v.to_str().ok())?
	};

// But model is required.
let model =
	if model_u8[0] == 0 { None }
	else {
		CStr::from_bytes_until_nul(model_u8.as_slice())
		.ok()
		.and_then(|v| v.to_str().ok())
	}?;

(DriveVendorModel::new handles normal whitespace trimming itself, so isn't strictly needed above.)

I don't know enough about the BOT protocol yet, but I suspect that some firmware/controllers may also return data in a short packet. In that case, we will need to check the CSW for the residual data bytes and, if the residual is > 0, call read_bulk again.

Yeah, me neither. Could be worth looking to see how the libcdio MMC driver handles it.

[joshstoik1]

@elmattic, how would you feel about having proper log support in Rip Rip?

I've been considering it for a while now, both as a replacement for the current -v/--verbose functionality and to give users a way to help me help them debug any random device weirdness.

Given how much heavy lifting your driver has to do, I'm thinking it would be better to get that in place sooner rather than later, but wanted to check with you first in case you had any reservations!

(As big a change as that would be, I don't think it should pose any particular rebase problems; the driver code is fairly well isolated now. 🤞)