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

Remove NCF_HAS_UI flag from Characteristics #94

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

rozmansi
Copy link
Contributor

@rozmansi rozmansi commented Aug 1, 2019

This flag indicates that the device has a supplemental DLL with a custom user interface. However, the TAP-Windows6 does not have any custom UI other than standard device registry properties.

Furthermore, HP Envy laptops have a built-in feature to turn off WiFi automatically when an Ethernet NIC gets connected. This feature actually checks if the Ethernet NIC is NCF_VIRTUAL to ignore it. Unfortunately, it literally checks Characteristics == NCF_VIRTUAL instead of (Characteristics & NCF_VIRTUAL) != 0. The end result is, an HP Envy laptop turns off WiFi when OpenVPN connects killing its only data link.

This commit remedies the HP Envy issue on the OpenVPN end. Thou it should be fixed at the source - namely HP. Preparations to report this issue to HP are underway.

Reference: https://docs.microsoft.com/en-us/windows-hardware/drivers/network/ddinstall-section-in-a-network-inf-file

This flag indicates that the device has a supplemental DLL with a custom
user interface. However, the TAP-Windows6 does not have any custom UI
other than standard device registry properties.

Furthermore, HP Envy laptops have a built-in feature to turn off WiFi
automatically when an Ethernet NIC gets connected. This feature actually
checks if the Ethernet NIC _is_ NCF_VIRTUAL to ignore it. Unfortunately,
it literally checks `Characteristics == NCF_VIRTUAL` instead of
`(Characteristics & NCF_VIRTUAL) != 0`. The end result is, an HP Envy
laptop turns off WiFi when OpenVPN connects killing its only data link.

This commit remedies the HP Envy issue on the OpenVPN end. Thou it
should be fixed at the source - namely HP. Preparations to report this
issue to HP are underway.

Reference: https://docs.microsoft.com/en-us/windows-hardware/drivers/network/ddinstall-section-in-a-network-inf-file
Signed-off-by: Simon Rozman <simon@rozman.si>
@selvanair
Copy link
Collaborator

selvanair commented Aug 1, 2019

The docs say NCF_HAS_UI indicates "Component supports a user interface (for example, the Advanced Page or a custom properties sheet)." We do have an advanced tab in device properties but if this refers to custom property pages handled by helper dlls, the patch makes perfect sense. Also, considering viscosity's version works without this flag, it does seem unnecessary.

As for HP envy, in reply to my suggestion to try NCF_VIRTUAL, JJK replied that the change did not help the wifi disconnection problem. Have you tested this on that laptop? However, this is irrelevant, as the HP envy silliness is not a good argument, nor necessary, to justify such a change. We can't possibly alter settings to suit an obscure laptop which may not be even in the market by the time this version is released.

@rozmansi
Copy link
Contributor Author

rozmansi commented Aug 2, 2019

As for HP envy, in reply to my suggestion to try NCF_VIRTUAL, JJK replied that the change did not help the wifi disconnection problem. Have you tested this on that laptop?

JJK tried to change it in the registry after the driver was installed and TAP adapter created, AFAIK.
I have changed it at the source: in the .inf file.

I have not tested it personally, as I do not possess any HP Envy laptop. I have compiled the driver using this commit and shipped it to an organization which uses HP Envy laptops exclusively. Because of this issue, the entire organization is unable to use OpenVPN nor eduVPN or any other solution based on TAP-Windows6 driver.
They confirmed that replacing with this driver fixed the problem.

However, this is irrelevant, as the HP envy silliness is not a good argument, nor necessary, to justify such a change. We can't possibly alter settings to suit an obscure laptop which may not be even in the market by the time this version is released.

It is just one of the arguments to make a change. Please, do not forget the original argument that our settings are wrong: TAP-Windows6 is not using a custom UI DLL and the NCF_HAS_UI flag is redundant and misleading.

Besides, I do not agree with an attitude "We're not fixing our driver. You change your laptops!"

@schwabe
Copy link

schwabe commented Aug 2, 2019

I looked a bit a the documentation and it seems that the flag we set (NCF_HAS_UI, 0x80) is required for custom property pages. But we don't have those.

I think this fixes a bug/cosmetic issue in our driver. That it also fixes HP's broken driver stack is just a nice side effect.

@selvanair
Copy link
Collaborator

Besides, I do not agree with an attitude "We're not fixing our driver. You change your laptops!"

Sorry if it came out that way -- I had no intention of implying anything like that. If the flag is redundant or wrongly set, we should get rid of it.

@mattock
Copy link
Member

mattock commented Aug 5, 2019

I'll merge this soon if nobody objects.

@cron2
Copy link
Contributor

cron2 commented Aug 5, 2019 via email

@mattock
Copy link
Member

mattock commented Aug 7, 2019

Everyone in today's community meeting agreed that we can merge this, so I'll do just that now 😄.

@mattock mattock merged commit 7f947f6 into OpenVPN:master Aug 7, 2019
@rozmansi rozmansi deleted the pending/characteristics branch September 11, 2019 07:39
@cron2
Copy link
Contributor

cron2 commented Oct 25, 2019

jjk confirmed that the test installer provided indeed fixes the HP Envy Wifi issue. So, merged, and actually tested as well :-)

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.

5 participants