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

use QSerialPortInfo to detect presence of a serial port (fix #836) #837

Conversation

heikobihr
Copy link
Contributor

Summary

  • ProviderRs232: checking for existence of device file does not work on windows.

  • instead: construct QSerialPortInfo from device name and check, if it is not null.

    When constructing QSerialPortInfo from device name, this constructor finds the
    relevant serial port among the available ones according to the port name name,
    and constructs the serial port info instance for that port.
    (https://doc.qt.io/qt-5/qserialportinfo.html#QSerialPortInfo-2)

Fixes: #836

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of web configuration, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing setups:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's body (e.g. Fixes: #xxx[,#xxx], where "xxx" is the issue number)

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated (docs/docs/en)
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

…-project#836)

* checking for existence of device file does not work on windows
* instead: construct QSerialPortInfo from device name and check, if it is not null
  When constructing QSerialPortInfo from device name, this constructor finds the
  relevant serial port among the available ones according to the port name name,
  and constructs the serial port info instance for that port.
  (https://doc.qt.io/qt-5/qserialportinfo.html#QSerialPortInfo-2)
@heikobihr heikobihr changed the title use QSerialPortInfo do detect presence of a serial port (fix #836) use QSerialPortInfo to detect presence of a serial port (fix #836) Jun 21, 2020
Copy link
Collaborator

@Lord-Grey Lord-Grey left a comment

Choose a reason for hiding this comment

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

Hi Heiko

Thanks for highlighting the difference between Linux and Windows, as well as the suggested solution.

Nevertheless, could you do me a favor and set the device in error with a failure text, if the device name is invalid, please?
i.e.

		if (!serialPortInfo.isNull())
		{
			...
		}
		else
		{
			QString errortext = QString("Invalid serial device name: [%1]!").arg(_deviceName);
			this->setInError(errortext);
			_preOpenDelayTimeOut = QDateTime::currentMSecsSinceEpoch() + _preOpenDelay;
			return false;
		}

Otherwise a user would wonder, why e.g. a non existing COM5 device is not working.

Thank you!

@hyperion-project
Copy link

Here is your new link to your workflow artifacts.

@Lord-Grey
Copy link
Collaborator

@heikobihr Thanks for the update.

I am currently in the process refactoring the Led-Devices's code which included the Rs232 one.
I have incorporated your input (plus some minor additions).

As it seems you have a serial device running with windows, would you mind testing my development branch from a Windows/Rs232 perspective?
That would be very much appreciated!

Many thanks in advance!

@Lord-Grey Lord-Grey self-requested a review June 23, 2020 20:43
@heikobihr
Copy link
Contributor Author

@Lord-Grey
Yes, I am running an adalight serial Port device with hyperion.ng on windows. I'll run a test of your development branch tonight.

@Paulchen-Panther
Copy link
Member

I would like it if the available serial devices were transferred to the JsonAPI. With this one could create a drop down field in the WebUI and select connected devices. Examples can be found in the V4L2 Grabber.

QSerialPortInfo Enumerator Example

@Lord-Grey
Copy link
Collaborator

Lord-Grey commented Jun 24, 2020

@Paulchen-Panther This is already available as part of my LedDevice Refactor Branch.
When I am back at my PC, I can quickly tell, how you could test it.
Edit: The backend implementation is available. The UI part is pending.

@heikobihr
Copy link
Contributor Author

@Lord-Grey
I tested your LedDevice Refactor Branch, successfully. Hyperion.ng connects to my adalight and shows rainbow swirl startup effect.

@brindosch
Copy link
Contributor

Thank you for contribution and testing.

@brindosch brindosch mentioned this pull request Jun 28, 2020
12 tasks
@brindosch brindosch merged commit e365a28 into hyperion-project:master Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProviderRs232: checking port's existence by device file does not work on windows
4 participants