Conversation
03be45c to
a399909
Compare
| /// The type of a CD disc | ||
| #[repr(u8)] | ||
| #[derive(Clone, Copy, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd, TryFromPrimitive)] | ||
| pub enum CdType { |
There was a problem hiding this comment.
Same idea as above: the type of the disk is a more global kind of thing than just reading a TOC. So consider moving this to a more general/global kind of place.
This fits in with the general idea that part of cdio-rust is about documenting and describing properties of CD/DVD/Blue Ray media independent of the specific components or functions inside MMC.
There was a problem hiding this comment.
A few goals here:
-
Use multiple specialized types if needed.
Say we had two methods, onecd_type()and otherdvd_type().
As the names imply, in this hypothetical example, the first method only returns CD types whereas the second returns DVD types.
Having two enums (CdType and DvdType) each of which only cover exactly what these methods return is cleaner than having one generalized type (DiscType) that each return only a subset of.
Ifcd_type()were to returnDiscTypewe would have a subset ofDiscType's variants that are not used.
Instead ifcd_type()returnedCdType, all variants a user knows exactly what outcomes are possible, and their code does not have to account for any dead paths (such as covering acd_type()returning a Dvd type).
Basically, we're leveraging the type system to declare the exact possible outcomes, at compile time. -
Place types as close to the routines that use them.
Currently, the only user of this type is that particular method.
If there were to be another method that perfectly covers the enum (in line with the above point), it can be moved to the parent module. Rust gives us the flexibility to do this without affecting the public API (see re-exports).
Let me know what you think.
There was a problem hiding this comment.
I am not sure I fully understand.
If the enumeration CdType is supposed to roughly correspond to https://github.com/libcdio/libcdio/blob/master/include/cdio/disc.h#L32-L57, then this gathers the disc modes in MMC listed in SEND CUESHEET.
See, for example, CD-DA Data Form in section 6.33.3.11 of https://www.13thmonkey.org/documentation/SCSI/mmc5r04.pdf and CD-FORM XA CD-I Form in the following example.
The TOC reading seems to need some of this disc form information. So there should be a common definition. And that definition should be tied to the specification somewhere.
Reusing parts of the enumeration elsewhere in a new local enumeration is fine.
|
Overall, looks good. The libcdio project as a project is a little different (but not uniquely different) in that it is not just about the code that provides useful routines, but that it also has a role in informing a standard or set of standards. I hope libcdio's organization or information in the C library can be preserved in cdio-rust so that it has that same informative benefit. |
Only `OsError` was used to represent MMC errors. Introduce `MmcError`, which also covers MMC errors by including sense data.
| BadParameter = libcdio_sys::driver_return_code_t_DRIVER_OP_BAD_PARAMETER, | ||
| } | ||
|
|
||
| /// Implemented MMC commands and their operation codes. |
There was a problem hiding this comment.
Thanks - this is a good start.
We should include a reference to a location inside a particular MMC spec to make it easier for people to verify compliance (or non-compliance ;-) with the spec.
READ TOC/PMA/ATIPREAD TOC/PMA/ATIP
|
Looks good - thanks for all of the changes. |
last_sector(): Returns the last valid sector of the disc.cd_text(): Returns the CD-TEXT of the disc.cd_type(): Returns the kind of CD of the present media.