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 friendlyName to PortInfo interface <superseeded - will delete upon confirmation of alternative fix> #105

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

Conversation

GazHank
Copy link
Contributor

@GazHank GazHank commented Jun 20, 2023

Update documentation per fix for serialport/node-serialport#2600

Adds friendlyName (Windows specific property) to Port definition

Signed-off-by: Gareth Hancock <64541249+GazHank@users.noreply.github.com>
@GazHank GazHank added the bug Something isn't working label Jun 20, 2023
@vercel
Copy link

vercel bot commented Jun 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 20, 2023 8:37am

@@ -61,6 +61,7 @@ interface PortInfo {
locationId: string | undefined;
productId: string | undefined;
vendorId: string | undefined;
friendlyName?: string | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is technically true 🤔

Should we make it always present on other platforms but undefined to be consistent?

Copy link
Contributor Author

@GazHank GazHank Jun 20, 2023

Choose a reason for hiding this comment

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

Hmm, I was trying to work out if we had set a precedent elsewhere... and the only example I could find was the LowLatency mode for Linux.... however upon a closer look, while it is only defined within the Linux specific interface definition, Windows actually returns False. So I think the PortStatus definition might need to be changed too :-(

Happy to pivot in either direction, but I think it would be good to be consistent in how we handle things across the package... Can you think of any other platform specific items we might need to consider at the same time?

Copy link
Member

@reconbot reconbot Jun 20, 2023

Choose a reason for hiding this comment

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

I would assume that eventually we can provide the friendly name on other systems. So I wouldn't consider it platform specific by design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll draft an alternative change that includes friendlyname within port info, and lowlatency within get info, but allows them to be undefined

@GazHank GazHank changed the title Add friendlyName to PortInfo interface Add friendlyName to PortInfo interface <superseeded - will delete upon confirmation of alternative fix> Jun 20, 2023
@GazHank
Copy link
Contributor Author

GazHank commented Jun 20, 2023

I've proposed an alternative set of changes to cover this and allow for the fact that lowLatency is returned as False when getting port info on Mac and Win. This resulted in changes to bindings-interface, bindings-cpp, bindings-mock, and the website... I don't think it requires any changes to the node-serialport repository since we have allowed undefined. Please let me know if I've missed anything, or if you would like to suggest any additional changes

ps - while I added lowLatency to the Get PortInfo for all platforms, I've left the Set Port Options as it was (linux specific)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants