Prefer manufacturer and model information from device over udev database

[sirhcel]

• This addresses Linux: serialport::available_ports() returns generic USB strings instead of actual descriptor values #134 and likely Wrong informations with Com port  #129
• Previously, our lookup preferred the information from udev's hardware database over the information provided by USB devices itself
  • The former is static an will for example provide the following for a SiliconLabs microcontroller eval board with a J-LINK interface

    $ cargo run --release --example list_ports
    [...]
      /dev/ttyACM0
        Type: USB
        VID:1366 PID:0105
         Serial Number: [...]
          Manufacturer: SEGGER
               Product: Zeppelin USB 3.0 Host controller
    [...]
    

  • Which even looks like faulty product information as this board has an USB 2.0 interface and is definitely no host controller
  • With this PR, the information from the device will be provided

    $ cargo run --release --example list_ports
    [...]
      /dev/ttyACM0
        Type: USB
        VID:1366 PID:0105
         Serial Number: [...]
          Manufacturer: Silicon Labs
               Product: J-Link OB
    [...]
    

• This change will break existing lookups due to different manufacturer and product information returned
  • We can state this in our changelog
  • Do we have the chance to prevent automatic updates by cargo update by releasing this change with a minor release?

[eldruin]

Thank you!
I would see a problem with this released in a patch release but I think it is fine for a minor release. The API does not break in the sense that it continues to compile, it just provides different strings. I would not see that information as SemVer-breaking.
I would like to avoid the unscape dependency, though. That crate seems rather abandoned.

[sirhcel]

Thanks for your feedback and starting the discussion @eldruin!

Thank you! I would see a problem with this released in a patch release but I think it is fine for a minor release. The API does not break in the sense that it continues to compile, it just provides different strings. I would not see that information as SemVer-breaking.

If a minor release prevents automatic updates by cargo update than this would be ideal. I don't feel like we should break bespoke routines for finding serialports with such a semiautomatic update. I tried to find a statement with respect to this in the Cargo docs but failed so far. Do you know if this is clearly stated somewhere?

If minor updates would happen automatically by Cargo, I would prefer, postponing this change to the next major release and put it behind a feature gate until then.

I would like to avoid the unscape dependency, though. That crate seems rather abandoned.

It definitely looks like being abandoned. Do you know of a worthy alternative? I've seen unescape-rs/#8 recommending unescaper, but it is released only under GPLv3 and therefor not compatible with the licensing of serialport-rs. Started the discussion for dual-licensing unescape-rs with hack-ink/unescaper#26.

On the other hand is the code from unescape sufficiently simple and according to the stats on crates.io it is still well used today. If there is serious trouble with this unescape, I would espect it to pop up as a RUSTSEC advisory which will show in CI. Maintaining our own unescape code and tests does not seem worth it to me in the first place. And as unescape as a dependency is used only internally, we could switch to another crate or own implementation at any time.

With that said, how would you prefer to continue with unescaping? I have no strong opinion yet.

[AurevoirXavier]

Hi, you can now use https://github.com/hack-ink/unescaper/releases/tag/v0.1.3.

:)

[sirhcel]

Thank you very much @AurevoirXavier for your quick response and action!

Now reality kicks in and I see that should have spent some thoughts on the compatibility of Rust versions and Cargo features beforehand. But I just built with the latest release of Rust locally and did not spot this incompatibility in the first place. 🤦 I will think about how we could get this resolved ...

[AurevoirXavier]

I will think about how we could get this resolved ...

It's already 2023, almost 2024. I highly recommend that you all upgrade to at least the 2021 edition.

[sirhcel]

It's already 2023, almost 2024. I highly recommend that you all upgrade to at least the 2021 edition.

This is indeed a good point @AurevoirXavier. One (if not the only) reason for sticking to this old MSRV is the support or the latest Yocto LTS (Kirkstone) release for being able to build serialport-rs in this environment. And to disclose my affiliation with it: I'm doing this at my day job and I am therefor opinionated on this. While writing this comment, I saw that the next LTS (Nanbield) is out since some days and ships with Rust 1.70. So a bigger bump is within reach.

But back to business: Our current MSRV already allows using the 2021 edition and I don't know for which reason (if any) we are still using the 2018 edition. If I had to guess I would assume that there was no focus on the edition when bumping MSRV in the past.

To my understanding, the Rust edition is a choice which just affects how serialport-rs is build and is independent of the edition chosen by its dependencies. What actually matters is the Version of Cargo we are using and its feature named-profiles got stabilized with 1.57.

And now the good news: Yocto Kirkstone's Rust support has been bumped to 1.59.0 this spring and updating our MSRV to this release would buy us out in the sort term. See this test and its build.

[sirhcel]

What's your opinion on bumping our MSRV to 1.59.0 with a minor release @jessebraham and @eldruin. I'm asking because during my testing I figure out that our examples no longer build with 1.56.1 due to an MSRV upgrade from a dev sub-dependency.

Its serialport > clap > clap_lex > os_str_bytes which bumped its MSRV with the release 6.6.0 to 1.60.1 with a minor release. I learned that the resolver automatically increases minor releases for versions 1.0.0 and above (1.0.0 == >=1.0.0 and <2.0.0). So even a small bump could be a breaking change in some fringe cases.

I also learned today that our MSRV is even behind the Rust release shipped with Debian stable. 😂 So I bet bumping our MSRV to 1.59.0 likely only affects people building our crate in a well aged Yocto environment and I'm inclined to being fine with bumping our MSRV with a minor release.

In the lesson above I also learned that our examples don't build with 1.56.1/1.59.0 because loopback.rs uses scoped threads. I did not expect that but looking into the CI builds revealed that we just build the library itself with MSRV there and the change to loopback.rs likely sneaked through because of CI currently not pointing this out. I'm okay with that as I expect people to build examples locally with a halfway up-to-date version of Rust and the ones building with a well-aged Yocto should be capable of sorting such issues out on their own.

[sirhcel]

I would like to avoid the unscape dependency, though. That crate seems rather abandoned.

Would you be fine with using unescaper @eldruin?

[jessebraham]

What's your opinion on bumping our MSRV to 1.59.0 with a minor release

I think that's reasonable. I had actually bumped this library to edition = "2021" previously, however this caused issues for some people (the Yocto folks, I think?) at that time. If this is no longer an issue then there is no reason not to bump the MSRV, IMO.

[sirhcel]

Updated MSRV and docs as well. Also extended the MSRV checks in CI to all platforms mentioned in README (with Linux getting some special treatment with respect to udev and Sysfs/musl).

Any objections against merging @eldruin and @jessebraham? And since we've accumulated some fixes since 4.2.2: Any objections with cutting a new release 4.3.0 then?

[eldruin]

Sorry, I was unexpectedly out for a few days.
Thank you for all your work and for going through all of this.
Bumping the MSRV to 1.59.0 sounds fine to me since we keep kirkstone working.
Using unscaper is fine since the license is now compatible. Thank you both for that!
About whether this crate would be updated by cargo update or not in some client code: it depends on their dependency specification. For example, if we release this in 4.3.0, if their Cargo.toml says serialport = "4" or serialport = "4.2", it will be updated. If it says serialport = "4.2.0" it will not be, for example. That is all documented here.
I would favor a 4.3.0 release indeed.

[sirhcel]

Had a quick and dirty check on our reverse dependencies on crates.io with this gist and it looks like there are only some pretty old versions of espflash using a MSRV below 1.59.0 which are currently downloaded only in homeopathic doses:

$ cargo run -- --version-requirement "^4" serialport
[...]
total dependencies: 144
dependencies on ^4: 99

infos for espflash
  Info { name: "espflash", num: "2.1.0", downloads: 14525, msrv: Some("1.70"), dependency_req: Some("^4.2.2") }
  Info { name: "espflash", num: "2.0.1", downloads: 21268, msrv: Some("1.65"), dependency_req: Some("^4.2.1") }
  Info { name: "espflash", num: "2.0.0", downloads: 7245, msrv: Some("1.65"), dependency_req: Some("^4.2.1") }
  Info { name: "espflash", num: "2.0.0-rc.4", downloads: 243, msrv: Some("1.65"), dependency_req: Some("^4.2.1") }
  Info { name: "espflash", num: "2.0.0-rc.3", downloads: 6482, msrv: Some("1.63"), dependency_req: Some("^4.2.0") }
  Info { name: "espflash", num: "2.0.0-rc.2", downloads: 187, msrv: Some("1.63"), dependency_req: Some("^4.2.0") }
  Info { name: "espflash", num: "2.0.0-rc.1", downloads: 240, msrv: Some("1.62"), dependency_req: Some("^4.2.0") }
  Info { name: "espflash", num: "1.7.0", downloads: 25226, msrv: Some("1.59"), dependency_req: Some("^4.2.0") }
  Info { name: "espflash", num: "1.6.0", downloads: 3127, msrv: Some("1.59"), dependency_req: Some("^4.2") }
  Info { name: "espflash", num: "1.5.1", downloads: 1727, msrv: Some("1.59"), dependency_req: Some("^4.1") }
  Info { name: "espflash", num: "1.5.0", downloads: 117, msrv: Some("1.59"), dependency_req: Some("^4.1") }
  Info { name: "espflash", num: "1.4.1", downloads: 724, msrv: Some("1.56"), dependency_req: Some("^4.1") }
  Info { name: "espflash", num: "1.4.0", downloads: 487, msrv: Some("1.56"), dependency_req: Some("^4.1") }
  Info { name: "espflash", num: "1.3.0", downloads: 896, msrv: Some("1.56"), dependency_req: Some("^4.0") }
  Info { name: "espflash", num: "1.2.0", downloads: 925, msrv: Some("1.56"), dependency_req: Some("^4") }
  Info { name: "espflash", num: "1.1.0", downloads: 861, msrv: None, dependency_req: None }
[...]
  Info { name: "espflash", num: "0.1.0", downloads: 163, msrv: None, dependency_req: None }
infos for kaleidoscope-focus
  Info { name: "kaleidoscope-focus", num: "0.1.0", downloads: 317, msrv: Some("1.59.0"), dependency_req: Some("^4.2") }
infos for rawzeo
  Info { name: "rawzeo", num: "0.0.1", downloads: 69, msrv: Some("1.66.1"), dependency_req: Some("^4.2.0") }

and there are no reverse dependencies to espflash which specify an MSRV. So I will sleep well with releasing this change as 4.3.0. What about cutting a release together with #141 @eldruin and @jessebraham?

[eldruin]

Looks good to me, thank you!
Yes, a new release with #141 as well sounds good.