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

Gracefully handle FT_OTHER_ERROR for FT_GetDeviceInfo #37

Merged
merged 4 commits into from
May 29, 2021

Conversation

CirrusNeptune
Copy link
Contributor

For devices that do not have a configured EEPROM attached, there are no USB string descriptors present. Under this condition, FT_GetDeviceInfo will return FT_OTHER_ERROR. This is the case for an Adafruit FT232H module with factory configuration and libftd2xx 1.4.24 (current x86_64 Linux release at time of writing).

Static analysis of this function indicates that all fields except serial_number and description are valid for FT_OTHER_ERROR. These string parameters are unmodified in this case.

This really seems to be a bug in FTDI's implementation. It also causes internal misbehaviors that affect the output of FT_GetDeviceInfoList (all zero fields).

For devices that do not have a configured EEPROM attached, there are no
USB string descriptors present. Under this condition, FT_GetDeviceInfo
will return FT_OTHER_ERROR.

Static analysis of this function indicates that all fields except
serial_number and description are valid for FT_OTHER_ERROR. These
string parameters are unmodified in this case.
@newAM
Copy link
Member

newAM commented May 23, 2021

For devices that do not have a configured EEPROM attached, there are no USB string descriptors present. Under this condition, FT_GetDeviceInfo will return FT_OTHER_ERROR. This is the case for an Adafruit FT232H module with factory configuration and libftd2xx 1.4.24 (current x86_64 Linux release at time of writing).

I had the same thing happen with my second FT232H from Adafruit. I assumed it was a one-off manufacturing error because the first one I have (purchased years earlier) worked fine. I guess they made a few with the same problem.

Static analysis of this function indicates that all fields except serial_number and description are valid for FT_OTHER_ERROR. These string parameters are unmodified in this case.

There may be some #ifdefs for non-linux platforms on the vendor side.
Is there a sane way to check that his holds true for all the targets?

There may be multiple conditions in this function that return FT_OTHER_ERROR which are not present on Linux, and the other fields may not always be valid for this return code. I think it is unlikely that this would be a problem, but this library has given me enough problems that I think it is necessary to double check assumptions here.

Alternatively, maybe we could sidestep the problem entirely and return the DeviceInfo in the Err type to let the user decide what to do?

Something along the lines of:

struct DeviceInfoError {
   device_info: DeviceInfo,
   status: FtStatus,
}

// add comment about known FT_OTHER_ERROR conditions here
fn device_info(&mut self) -> Result<DeviceInfo, DeviceInfoError> {
    let mut device_type: u32 = 0;
    let mut device_id: u32 = 0;
    let mut serial_number: [i8; STRING_LEN] = [0; STRING_LEN];
    let mut description: [i8; STRING_LEN] = [0; STRING_LEN];
    trace!("FT_GetDeviceInfo({:?}, _, _, _, _, NULL)", self.handle());
    let status: FT_STATUS = unsafe {
        FT_GetDeviceInfo(
            self.handle(),
            &mut device_type,
            &mut device_id,
            serial_number.as_mut_ptr() as *mut c_char,
            description.as_mut_ptr() as *mut c_char,
            std::ptr::null_mut(),
        )
    };
    let (vid, pid) = vid_pid_from_id(device_id);
    let device_info = DeviceInfo {
        port_open: true,
        speed: None,
        device_type: device_type.into(),
        product_id: pid,
        vendor_id: vid,
        serial_number: slice_into_string(serial_number.as_ref()),
        description: slice_into_string(description.as_ref()),
    };
    
    if status != 0 {
        Err(DeviceInfoError { device_info, status: status.into() })
    } else {
        Ok(device_info)
    }
}

What are your thoughts here?

This really seems to be a bug in FTDI's implementation. It also causes internal misbehaviors that affect the output of FT_GetDeviceInfoList (all zero fields).

Yeah... The vendor library is pretty bad on linux... I had to add list_devices_fs to work around bugs in that same function.

src/lib.rs Outdated
Comment on lines 616 to 619
serial_number: match status {
FT_OK => slice_into_string(serial_number.as_ref()),
_ => String::default(),
},
Copy link
Member

Choose a reason for hiding this comment

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

slice_to_string should handle empty strings.

Suggested change
serial_number: match status {
FT_OK => slice_into_string(serial_number.as_ref()),
_ => String::default(),
},
serial_number: slice_into_string(serial_number.as_ref()),

@CirrusNeptune
Copy link
Contributor Author

Alternatively, maybe we could sidestep the problem entirely and return the DeviceInfo in the Err type to let the user decide what to do?

The main issue I'm facing isn't calls to device_info from my application, rather the way libftd2xx-rs calls it when converting from generic devices to specific devices. The OTHER_ERROR case could potentially be handled there instead.

I'm not really equipped for Windows development at the moment, but it's a good point that there could be a completely separate set of OTHER_ERROR semantics there.

@CirrusNeptune
Copy link
Contributor Author

Are the ffi types for the FT_STATUS codes supposed to be c_int on Windows? I thought the data type was c_ulong.

@newAM
Copy link
Member

newAM commented May 24, 2021

The main issue I'm facing isn't calls to device_info from my application, rather the way libftd2xx-rs calls it when converting from generic devices to specific devices. The OTHER_ERROR case could potentially be handled there instead.

Ah, good point.
Let me think on this for a bit, the first few ideas coming to mind are not good ones.

Are the ffi types for the FT_STATUS codes supposed to be c_int on Windows? I thought the data type was c_ulong.

They are an enum in the original ftd2xx.h which bindgen casts to different types for the different platforms. Apparently Windows handles this differently than Linux ¯\_(ツ)_/¯? I was still learning the language back then so I did not dig into it as much as I should have. There's some cross-platform types here to work around it: https://github.com/newAM/libftd2xx-rs/blob/main/src/errors.rs#L136

@CirrusNeptune
Copy link
Contributor Author

Ahh, good to know about the types. Didn't realize the bindings are generated.

I tried making sense of the Windows implementation of GetDeviceInfo, but the bulk of it is done in kernel mode and I don't really have to willpower to untangle that mess. 😅

@newAM
Copy link
Member

newAM commented May 24, 2021

I tried making sense of the Windows implementation of GetDeviceInfo, but the bulk of it is done in kernel mode and I don't really have to willpower to untangle that mess. 😅

No worries, my willpower to develop on Windows is low too 👍

In the meantime, if you want to put valid settings on your EEPROM I used this python CLI tool to put valid settings onto mine: https://eblot.github.io/pyftdi/eeprom.html

@newAM newAM added the bug Something isn't working label May 24, 2021
@CirrusNeptune
Copy link
Contributor Author

Cool! Thanks for that. Though I'm still hoping users of my application would be able to use their unprogammed devices if at all possible.

I reached out to FTDI to set the record straight on what FT_OTHER_ERROR actually means. I hope that this code can be made into a stable, well documented API semantic.

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented May 25, 2021

Got a response back from FTDI.

Their current position is WONTFIX due to their inability to reproduce the issue and lack of reports from other customers. This is in spite of me providing detailed information about what causes the return code, concerns about the ambiguity of such an error code in the first place, and a parsed dump of my USB descriptors.

Though I've never had much luck dealing with "Technical Support Engineers" in the past, so I can't say I'm surprised.

@newAM
Copy link
Member

newAM commented May 26, 2021

That's unfortunate, but I am not surprised. I submitted some documentation bugs to them a while ago (practically free to fix) and the same issues are still outstanding a year later.

Ideas:

  • unsafe fn into_ft232h_unchecked
  • Figure out a way to get the PID/VID on Windows without reading the EEPROM (should be available in headers) and make a into based off of that.
  • Make the try_from call the FFI for device type and ignore FT_OTHER_ERROR only of the device type matches the expected.

@CirrusNeptune
Copy link
Contributor Author

* Figure out a way to get the PID/VID on Windows without reading the EEPROM (should be available in headers) and make a `into` based off of that.

What's the behavior on Windows out of curiosity? Do none of the fields get returned for an unprogrammed EEPROM? What's the return code in that case?

@newAM
Copy link
Member

newAM commented May 27, 2021

What's the behavior on Windows out of curiosity? Do none of the fields get returned for an unprogrammed EEPROM? What's the return code in that case?

No idea unfortunately, I loaded mine with a valid EEPROM without ever using it on Windows.
I would test it out, but I have bricked a FTDI device before with (unintentional) invalid EEPROM settings.

@CirrusNeptune
Copy link
Contributor Author

I just tried out my board on Windows. FT_GetDeviceInfo actually succeeds and returns all fields. Obviously these aren't coming from the USB descriptors, so perhaps they're fallback constants hard-coded into the driver???

Serial: FTXZ7WQM
Description: FT232H

I tried it again on Linux just in case it sneakily programmed my device for me. Nope, still no descriptors, still FT_OTHER_ERROR

@CirrusNeptune
Copy link
Contributor Author

Also had a look at the macOS version. It's essentially the same as the Linux version, returning FT_OTHER_ERROR when libusb_get_string_descriptor_ascii fails.

@newAM
Copy link
Member

newAM commented May 27, 2021

I just tried out my board on Windows. FT_GetDeviceInfo actually succeeds and returns all fields. Obviously these aren't coming from the USB descriptors, so perhaps they're fallback constants hard-coded into the driver???

Serial: FTXZ7WQM
Description: FT232H

I tried it again on Linux just in case it sneakily programmed my device for me. Nope, still no descriptors, still FT_OTHER_ERROR

Can you hexdump your eeprom with something like pyftdi? If there are valid values in there we could just dump the EEPROM to check the device type.

This is definitely a bug in libftd2xx now though... Platform specific behavior for the same function is never desirable.

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented May 27, 2021

Oh! Looks like my EEPROM is programmed after all. Is it malformed in some way?

000000   00 00 03 04 14 60 00 09 80 32 08 00 00 00 a0 12   .....`...2......
000010   b2 0e c0 12 00 00 00 00 00 00 00 00 67 00 00 00   ............g...
000020   12 03 41 00 64 00 61 00 66 00 72 00 75 00 69 00   ..A.d.a.f.r.u.i.
000030   74 00 0e 03 46 00 54 00 32 00 33 00 32 00 48 00   t...F.T.2.3.2.H.
000040   12 03 46 00 54 00 58 00 5a 00 37 00 57 00 51 00   ..F.T.X.Z.7.W.Q.
000050   4d 00 02 03 00 00 00 00 00 00 00 00 00 00 00 00   M...............
000060   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00   ................
000070   00 00 00 00 00 00 00 00 00 00 00 00 00 00 f8 bb   ................
000080   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................
000090   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................
0000a0   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................
0000b0   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................
0000c0   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................
0000d0   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................
0000e0   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................
0000f0   ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff   ................

EDIT: Ah-ha! The string indices are all 128 bytes ahead of the actual data. As if it's expecting to be put on a 128-byte EEPROM and not a 256-byte one. So yeah, this would appear to be an Adafruit issue. I can only assume the Windows driver simply does its own EEPROM read and assumes it's 128 bytes and masks the address accordingly.

@CirrusNeptune
Copy link
Contributor Author

How would you feel about adding this to FtdiCommon? Then TryFrom could just use this instead.

Type constants are from pyftdi. It doesn't have exhaustive coverage of DeviceType though, so this could be problematic for those other devices.

/// Identify device type with a single EEPROM read.
///
/// # Example
///
/// ```no_run
/// use libftd2xx::{Ftdi, FtdiCommon};
///
/// let mut ft = Ftdi::new()?;
/// let dev_type = ft.device_type()?;
/// println!("Device type: {:?}", dev_type);
/// # Ok::<(), libftd2xx::FtStatus>(())
/// ```
fn device_type(&mut self) -> Result<DeviceType, FtStatus> {
    match self.eeprom_word_read(0x3)? {
        0x0200 => Ok(DeviceType::FTAM),
        0x0400 => Ok(DeviceType::FTBM),
        0x0500 => Ok(DeviceType::FT2232C),
        0x0600 => Ok(DeviceType::FT232R),
        0x0700 => Ok(DeviceType::FT2232H),
        0x0800 => Ok(DeviceType::FT4232H),
        0x0900 => Ok(DeviceType::FT232H),
        0x1000 => Ok(DeviceType::FT_X_SERIES),
        _ => Err(FtStatus::NOT_SUPPORTED)
    }
}

@CirrusNeptune
Copy link
Contributor Author

Managed to fill in the last four from an obvious switch statement in FT_GetDeviceInfo, but FT100AX is a mystery because nothing in FT_GetDeviceInfo maps to it.

I put the panic in there to match the one in From<u32>

fn device_type(&mut self) -> Result<DeviceType, FtStatus> {
    Ok(match self.eeprom_word_read(0x3)? {
        0x0200 => DeviceType::FTAM,
        0x0400 => DeviceType::FTBM,
        // ??? => DeviceType::FT100AX,
        0x0500 => DeviceType::FT2232C,
        0x0600 => DeviceType::FT232R,
        0x0700 => DeviceType::FT2232H,
        0x0800 => DeviceType::FT4232H,
        0x0900 => DeviceType::FT232H,
        0x1000 => DeviceType::FT_X_SERIES,
        0x1700 => DeviceType::FT4222H_3,
        0x1800 => DeviceType::FT4222H_0,
        0x1900 => DeviceType::FT4222H_1_2,
        0x2100 => DeviceType::FT4222_PROG,
        value => panic!("unknown device type in EEPROM: {}", value),
    })
}

@newAM
Copy link
Member

newAM commented May 27, 2021

How would you feel about adding this to FtdiCommon? Then TryFrom could just use this instead.

I think the vendor function may actually be keying off of the VID/PID and not the EEPROM (maybe?)
Ideally we can try the vendor function first and use EEPROM reads as a fallback.

I put the panic in there to match the one in From

I wrote this back when I was still learning rust, I should probably fix that at some point 😨, ideally we can return an error instead.

Something along the lines of:

/// Identify device type.
///
/// This will attempt to identify the device using the the [`device_info`]
/// method, if that method fails it will then try to determine the device type
/// from the value stored in the EEPROM.
/// If the EEPROM value does not match a known device this function returns
/// [`FtStatus::OTHER_ERROR`], though there may be other conditions in the
/// vendor driver that also return this code.
///
/// This is not a native function in `libftd2xx`, this works around a bug in
/// `libftd2xx`, see https://github.com/newAM/libftd2xx-rs/pull/37 for more
/// information.
///
/// # Example
///
/// ```no_run
/// use libftd2xx::{Ftdi, FtdiCommon};
///
/// let mut ft = Ftdi::new()?;
/// let dev_type = ft.device_type()?;
/// println!("Device type: {:?}", dev_type);
/// # Ok::<(), libftd2xx::FtStatus>(())
/// ```
fn device_type(&mut self) -> Result<DeviceType, FtStatus> {
    if let Ok(info) = self.device_info() {
        Ok(info.device_type)
    } else {
        match self.eeprom_word_read(0x3)? {
            0x0200 => DeviceType::FTAM,
            0x0400 => DeviceType::FTBM,
            // ??? => DeviceType::FT100AX,
            0x0500 => DeviceType::FT2232C,
            0x0600 => DeviceType::FT232R,
            0x0700 => DeviceType::FT2232H,
            0x0800 => DeviceType::FT4232H,
            0x0900 => DeviceType::FT232H,
            0x1000 => DeviceType::FT_X_SERIES,
            0x1700 => DeviceType::FT4222H_3,
            0x1800 => DeviceType::FT4222H_0,
            0x1900 => DeviceType::FT4222H_1_2,
            0x2100 => DeviceType::FT4222_PROG,
            _ => Err(FtStatus::OTHER_ERROR)
        }
    }
}

Then for each specific device that implements FtdiCommon we can change the const DEVICE_TYPE to an infallible method, e.g.

impl FtdiCommon for Ft232h {
    fn handle(&mut self) -> FT_HANDLE {
        self.handle
    }

    fn device_type(&mut self) -> Result<Ftdi, FtStatus> {
        Ok(DeviceType::FT232H)
    }
}

What do you think?

@CirrusNeptune
Copy link
Contributor Author

I think the vendor function may actually be keying off of the VID/PID and not the EEPROM (maybe?)
Ideally we can try the vendor function first and use EEPROM reads as a fallback.

I like this idea. The EEPROM read is essentially how pyftdi does device selection, seems like a good fallback.

I'll update the PR in a bit.

@newAM newAM merged commit 03f4445 into ftdi-rs:main May 29, 2021
@CirrusNeptune CirrusNeptune deleted the getdeviceinfo-other-error branch May 29, 2021 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants