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

Update libusb to version 1.0 #673

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kenspeckle1
Copy link

Fixes #191
I reimplemented the usb code for libusb1.0.
Currently it uses https://github.com/kenspeckle1/libnfc/tree/update_libusb10_branch as base which in turn updates the libusb10 branch to match the master branch. As such it would be best if PR #672 would be merged before this or the libusb10 branch would be updated in some other way.

A review and help with testing would be greatly appreciated. I only have access to an ACR122U device and a linux pc so I am unable to test other devices or OSes.

@hank9999
Copy link

hank9999 commented Sep 2, 2023

I tried it on Windows 11 with PN532 UART (built on Visual Studio 2022)
It seem that it works fine

@kenspeckle1
Copy link
Author

@hank9999 thank you for testing. I don't think that is project is maintained anymore. Just one pull request was merged over the last two years and no real interactions from the lead developers. I will leave this pull request open for anyone who wants to create a libusb1.0 fork and in the hopes that one day, this project will become active again

@hank9999
Copy link

hank9999 commented Oct 1, 2023

@hank9999 thank you for testing. I don't think that is project is maintained anymore. Just one pull request was merged over the last two years and no real interactions from the lead developers. I will leave this pull request open for anyone who wants to create a libusb1.0 fork and in the hopes that one day, this project will become active again

Thank you for replying. Yep, the project seems that it is not maintained, hope someone have time to merge the changes.

@neomilium
Copy link
Member

You are right, this project is not actively maintained anymore, but still working as expected.

@kenspeckle1 Could you rebase your work on top of main branch in order to ease rework then merge it?

@kenspeckle1 kenspeckle1 changed the base branch from libusb10 to master October 30, 2023 17:45
@kenspeckle1
Copy link
Author

kenspeckle1 commented Oct 30, 2023

Hi @neomilium, nice to see, that the project is not completely abandoned.
I changed to target branch to nfc-tools:master and updated this branch.

@kenspeckle1 kenspeckle1 changed the title WIP: Update libusb to version 1.0 Update libusb to version 1.0 Oct 30, 2023
Copy link
Member

@neomilium neomilium left a comment

Choose a reason for hiding this comment

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

Requested changes:

  • The merge commit should not be as this PR should be rebased on top of master.
  • «Restore NEWS.md» commit should be squashed with the commit that wrongly removes it.
  • «Bugfixes» should be squashed with the commit that introduces the bugs.
  • «Apply styles for changed files» should be integrated within development commits

@neomilium
Copy link
Member

As the switch between libusb 0.1 to 1.0 is a huge step and can potentially introduces bugs, we need to have a perfectly clean git history. BTW, the review will be more efficient and reliable if all commits are short, understandable and well described in git log.
Thanks!

@kenspeckle1
Copy link
Author

kenspeckle1 commented Nov 6, 2023

I do not understand why we can't just squash the commit when merging. My work is based on the 10 year old libusb10 branch with 19 commits from both peugeot and doegox. Wouldn't it be cleaner to squash these commits as well?

@kenspeckle1
Copy link
Author

Also, I will try to fix the history next weekend

@kenspeckle1
Copy link
Author

@neomilium Sadly it took me a bit longer than anticipated but I squashed all commits into one single commit

@neomilium
Copy link
Member

With the commit squash we loose the authoring, and all well-written atomic commits.

AFAIR, some commits were written by @doegox , maybe some by @smortex .

From my point of view its not acceptable and not more reviewable than before, when we had a big and dirty merge commit.

@doegox , @smortex, maybe @iwamatsu and any available authors, as I'm not comfortable with this PR due to the impact what are your position?

@doegox
Copy link
Member

doegox commented Nov 29, 2023

historical branch of libusb10 has commits mostly by myself, one by @LudovicRousseau and one by Lucien Judert.
The problem with a massive squash, besides authorship, is that it's impossible to track what was really done by @kenspeckle1
For example I don't see any trace of commit 5f71a79 of Lucien in this PR.
Maybe there was a good reason to discard it, maybe it was forgotten, hard to say when we have a single squashed commit, and therefore hard to review.
And that's just one commit, I didn't spend time comparing every previous commit with this squash.

@ikspress
Copy link
Contributor

ikspress commented Feb 16, 2024

Your code violates the coding style. Additionally, the mingw-cross-compile.sh script seems redundant after this PR, so I suggest deleting it. Also, MSYS2 and Linux seems to have an issue with the location of libusb.h, which uses <libusb-1.0/libusb.h> instead of <libusb.h>. Can you please address these issue?
I have modified your PR to make it compatible with the MSYS2 MINGW64 environment. You can follow my patch to make the necessary adjustments to your PR.

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.

Switch from libusb-0.1 to libusb-1.0
5 participants