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

Add more ftdi wrappers #14

Merged
merged 3 commits into from
Aug 12, 2021
Merged

Add more ftdi wrappers #14

merged 3 commits into from
Aug 12, 2021

Conversation

geomatsi
Copy link
Contributor

This PR adds the following new Rust wrappers for libftdi functions:

  • usb_close
  • set_bitmode and BitMode enum
  • set_flow_control and FlowControl enum

This is an updated version of #2.

Regards,
Sergey

@tanriol
Copy link
Owner

tanriol commented Aug 11, 2021

What's the purpose of the usb_close wrapper? The Device automatically calls usb_close on drop. I'd prefer to avoid exposing a state where everything looks as if the device is operational except that it has been closed, so most operations either do nothing or error out.

@tanriol
Copy link
Owner

tanriol commented Aug 11, 2021

Also, sorry, I've found a few commits I forgot to push last time that seem to conflict with some of your changes :-(

@geomatsi
Copy link
Contributor Author

Hi @tanriol

I dropped commit adding usb_close. Indeed, now it is not needed since ftdi_usb_close has been added into Drop implementation. Besides, I rebased my PR on top of your latest changes and force-pushed my updates.

Regards,
Sergey

@tanriol
Copy link
Owner

tanriol commented Aug 11, 2021

I'll look at this in more detail tomorrow.

One question I'm not sure about is whether the bitmodes are close enough to actually treat them as the same object. For example, IIRC, some mode(s) had hardcoded baud rates that ignored the set_baud_rate command. Does it really make sense to have that command available with such a mode?

For an even more interesting example, does it really make sense for the MPSSE mode to implement Write, or should there be a separate method for sending MPSSE commands?

Oh, and it would be nice if the names used were more Rust-style than copied-straight-from-libftdi-style. FlowControl::SIO_XON_XOFF_HS looks kinda weird to me. Not even speaking about BitMode::RESET, which does... what exactly? Hold the device inoperable in reset until there's a command to change mode to some other?

@geomatsi
Copy link
Contributor Author

Hi,

Adding these functions I assume that ftdi-rs is a wrapper that exposes libftdi interface w/o API changes. So BitMode is just a wrapper for libftdi enum ftdi_mpsse_mode. Concerning the naming, I agree that SIO_DISABLE_FLOW_CTRL does not look 'rusty'. However SioDisableFlowCtrl looks even worse to me. On the other hand, I think it makes sense to keep notation close to libftdi origins to simplify things for users. So let me know if you have something reasonable in mind. I will update PR according to your suggestion.

Regards,
Sergey

@tanriol
Copy link
Owner

tanriol commented Aug 12, 2021

ftdi-rs tries to make an interface for libftdi that would feel native in Rust. Yes, I'd prefer to keep it close enough that libftdi users can read code and write code with documentation without problems, but there's no requirement to expose it without API changes.

For flow control I'd suggest FlowControl::{Disabled, RtsCts, DtrDsr, XonXoff}. Not really sure about bitmode, so let's go with BitMode::Mpsse etc. for now and fix this if needed in later versions.

By the way, do we need to reset any of these when opening the device, or does libftdi already set them to some known initial state?

Add wrappers for libftdi flow control defines. Add set_flow_control
wrapper for ftdi_setflowctrl function.
Add wrappers for libftdi bitmode defines. Add set_bitmode
wrapper for ftdi_set_bitmode function.
@geomatsi
Copy link
Contributor Author

Hi,
I force-pushed the updated MR. Naming conventions updated according to your suggestions. Besides, additional cleanup commit has been added: cargo formatting applied.

I can not comment if any reset operations are needed in general on init. I guess if a specific mode has its own quirks, then they can be applied by the application using that mode. In my case I init MPSSE mode according to FTDI ANs.

Regards,
Sergey

@tanriol tanriol merged commit 9ab2972 into tanriol:master Aug 12, 2021
@tanriol
Copy link
Owner

tanriol commented Aug 12, 2021

I've merged your MR and added a number of fixups on top. It would also be nice to provide documentation on its usage, but that will have to wait for later.

@geomatsi geomatsi deleted the wip-add-more-wrappers branch August 12, 2021 19:50
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.

2 participants