search

ftdi-rs / libftd2xx

Rust safe wrappers for the libftd2xx drivers
MIT License
32 stars 15 forks source link

RFC: move mpsse helpers to the separate ftdi-mpsse crate #48

Closed geomatsi closed 2 years ago

geomatsi commented 3 years ago

Hi @newAM

As a follow-up to our discussion, this PR proposes to move all the MPSSE helpers and definitions into the separate ftdi-mpsse crate. The purpose is to allow reuse of MPSSE definitions and command builder outside libftd2xx-rs, e.g. in ftdi-rs and ftdi-embedded-hal.

Notes:

I would suggest to move new crate to your github space in the case if proposed approach looks reasonable to you. Well, obviously after all the review comments are addressed.

Thoughts ? Comments ?

Regards, Sergey

newAM commented 3 years ago

Thanks for the PR! I am pretty busy right now with work, I will review it this weekend.

geomatsi commented 2 years ago

Hi @newAM

I created new ftdi-mpsse repository in the new space and posted the accompanying pull-request: https://github.com/ftdi-rs/ftdi-mpsse/pull/1. At the moment my goal is to finalize the following 3 connected PRs:

I looks like the first portion of your review comments for this RFC PR has been addressed. So it is a good time to rebase and re-organize this PR. Are you OK with force-push here ?

Regards, Sergey

newAM commented 2 years ago

Are you OK with force-push here ?

Yup, that's fine by me!

geomatsi commented 2 years ago

This MR and its accompanying MRs have been updated according to review comments:

The last MR has been simplified after the new crate definitions have been re-exported by libftd2xx-rs. I think these three MRs are cleaned-up and ready for the next review round.

After we are done with review, I think the upcoming activities can be ordered as follows:

Thoughts ? Comments ?

Regards, Sergey

newAM commented 2 years ago

Work has me busy again, I will be sure to take a look this weekend!

newAM commented 2 years ago

I opened a PR here to add CI the ftdi-mpsse: https://github.com/geomatsi/ftdi-mpsse-1/pull/1 Pending your feedback on that I think it is ready for a v0.1.0 release.

merge ftd2xx-embedded-hal#35 , move repository to ftdi-rs organization and then and publish next version after setting next version for libftd2xx-rs in its dependencies

Is the plan to make that crate generic for libftdi and libftd2xx? I was thinking your original name (ftdi-embedded-hal) would be better for the generic HAL. Then I can just archive fd2xx-embedded-hal when the generic HAL is released.

geomatsi commented 2 years ago

I opened a PR here to add CI the ftdi-mpsse: geomatsi/ftdi-mpsse-1#1 Pending your feedback on that I think it is ready for a v0.1.0 release.

Great ! All merged. One more PR is submitted: updated list of authors and fixed CI.

merge ftd2xx-embedded-hal#35 , move repository to ftdi-rs organization and then and publish next version after setting next version for libftd2xx-rs in its dependencies

Is the plan to make that crate generic for libftdi and libftd2xx? I was thinking your original name (ftdi-embedded-hal) would be better for the generic HAL. Then I can just archive fd2xx-embedded-hal when the generic HAL is released.

Initially I have been experimenting with my ftdi-embedded-hal adding support for both libftdi-rs and libftd2xx-rs backends. Meanwhile your HAL implementation is more mature. So I suggest to copy it to the ftdi-rs organization under new name ftdi-embedded-hal. Then we can work on making it generic for both backends.

Regards, Sergey

geomatsi commented 2 years ago

BTW, do you mind if I create copies of libftd2xx-rs and ftd2xx-embedded-hal (renamed to ftdi-embedded-hal) in ftd-rs space ?

newAM commented 2 years ago

Initially I have been experimenting with my ftdi-embedded-hal adding support for both libftdi-rs and libftd2xx-rs backends. Meanwhile your HAL implementation is more mature. So I suggest to copy it to the ftdi-rs organization under new name ftdi-embedded-hal. Then we can work on making it generic for both backends.

Sounds good to me.

BTW, do you mind if I create copies of libftd2xx-rs and ftd2xx-embedded-hal (renamed to ftdi-embedded-hal) in ftd-rs space ?

For libftd2xx-rs I will move this repo in when this is merged.

geomatsi commented 2 years ago

Ok. I updated the link to ftdi-mpsse. It is still git repo until ftdi-mpsse is not published. Let me know what you expect from me before you are ready to get this PR merged.

newAM commented 2 years ago

Ideally a release of ftdi-mpsse can be made before this gets merged. Do you have a crates.io account? I couldn't find your name there.

newAM commented 2 years ago

I found out that moving repos on github is actually very non-destructive and wont impact this PR so I moved the repo over to ftdi-rs.

geomatsi commented 2 years ago

Ideally a release of ftdi-mpsse can be made before this gets merged. Do you have a crates.io account? I couldn't find your name there.

No, not yet. So if you have time to publish it, that would be great. If not, then I will register on crates.io and learn how to do it.

newAM commented 2 years ago

No, not yet. So if you have time to publish it, that would be great. If not, then I will register on crates.io and learn how to do it.

Sounds good, I will do that now.

newAM commented 2 years ago

https://github.com/ftdi-rs/ftdi-mpsse/actions/runs/1275624692

The release is in the pipeline now :tada:

Edit: and it is live! https://crates.io/crates/ftdi-mpsse

If/when you get a crates.io account let me know and I will add you as an owner of the repos in ftdi-rs.

geomatsi commented 2 years ago

Force-pushed after updating the first commit in the PR: added ftdi-mpsse version "^0.1".