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

HID minidriver module? #69

Closed
hansmbakker opened this issue Jun 26, 2019 · 48 comments
Closed

HID minidriver module? #69

hansmbakker opened this issue Jun 26, 2019 · 48 comments

Comments

@hansmbakker
Copy link

hansmbakker commented Jun 26, 2019

This issue is a follow-up to #13.

Currently there is a Virtual HID Framework module but it does not support UMDF since VHF only supports KMDF.

@samtertzakian could you please comment on these two questions:

  • are there any plans to add a HID minidriver (not VHF based) module to provide a solution that supports UMDF?
  • if not, would you be interested in collaborating on a PR that adds such a module? I have not written one yet, but I have written a driver based on the vhidmini2 sample that I would like to refactor so that it makes use of DMF. Part of that refactoring would be the creation of a HID minidriver module that could become part of DMF.

I am not a professional driver developer (I am a C# developer), so some best practices that are used in the DMF codebase might be missing from my code.

@samtertzakian
Copy link
Member

Hi, Hans,
Let me investigate the answers to your questions. Certainly, if there are no plans for UMDF support for VHF, one of us can collaborate with you. Lets leave this issue open until I get the answers to your questions and we decide how to address your issue. The fact of the matter is that refactoring for DMF is usually not a difficult thing to do. So, if I cannot get answers quickly, I may just do that and send it to you. I will try to get back to you in a couple of days if not sooner.

@hansmbakker
Copy link
Author

Hi Sam,
thank you for your supportive reply! If I can do something to help, please let me know.

@AndrewYangMSFT
Copy link

@hansmbakker, can you elaborate on what specific use scenario of HID output reports you think are not supported? Output reports are supported by IOCTL_HID_WRITE_REPORT in HID mini drivers and VHF callback EVT_VHF_ASYNC_OPERATION in VHF source drivers.

@hansmbakker
Copy link
Author

hansmbakker commented Jun 27, 2019

Oh, in that case it is my misunderstanding and then the only limitation imposed by VHF is that it is KMDF-only. Thank you for explaining! I'll update the opening post.

@AndrewYangMSFT
Copy link

@hansmbakker Thanks for the reply. Sam and you are right that UMDF support for VHF is a known limitation right now. However, we are actually evaluating that for the coming Windows release. Stay tuned.

@hansmbakker
Copy link
Author

@AndrewYangMSFT that is really good to hear! I know Microsoft presented VHF as the recommended alternative to HID minidrivers but I went for the latter because of its limitation to KMDF.

While I really welcome the VHF for UMDF evaluation, I also suspect that will be a long(er)-term solution since it is being evaluated. In this github issue I meant to ask whether there are any nearer-term plans for a solution that works for UMDF, and if not, whether I could help with that in collaboration with you.

@samtertzakian
Copy link
Member

Hi, Hans,
I hope send an update for your query by end of this week. Thank you for your patience. I am still evaluating the options.

@samtertzakian
Copy link
Member

Hi, Hans,

Because there is no clear time line about VHF UMDF support, I am going to try make a Module that acts like vhidmini. I can do it pretty easily. I will try to send a branch with that code that you can use to verify on your end and fix any issues you find. After you are satisfied then either you or i can submit the branch. I think that is the most efficient way to get you up and running. It also provides a nice sample that shows how to convert from a driver to a Module.
Thank you for your patience.

Disclaimer: This is the plan at this point. There is a possibility something may come up or there might be an issue that cause a delay.

@hansmbakker
Copy link
Author

Hi Sam,
that sounds great, looking forward to it!

@samtertzakian
Copy link
Member

Hi, Hans,
I have not forgotten about this issue. I am trying to get this task done ASAP. I will let you know more soon.

@hansmbakker
Copy link
Author

Thank you for keeping me updated, Sam!

@samtertzakian
Copy link
Member

Hello, Hans,
I am making good progress. I think I am about 50% done, maybe more. It is taking longer than I expected due to other tasks that I need to do. It takes a bit of time to properly separate the generic code from the specific code of the example.(It is not just a matter of copying code from sample into a Module.) However, I am happy to be doing it because it is a good example of how to perform such tasks in DMF. Also, this sample will include a much needed DMF User-mode sample. Thank you for your patience. As soon as I have something running, i will make a branch that you can look at and perhaps contribute to if you wish to.

@samtertzakian
Copy link
Member

Hello, Hans,

I am happy to report that I have created a branch with an initial code for the DMF version of VHIDMINI2.
The code in this branch allows both a Kernel and User-mode version of a virtual HID device to be created. (You wanted to make a virtual HID device in User-mode.)

Branch: https://github.com/microsoft/DMF/tree/personal/satertza/vhidmini2

The code basically works but there are some issues with the test application that comes with the original MSDN sample. It runs but shows a few errors. Also, the code needs to be cleaned up for style and formatting as well as general clean up. I have single stepped the code and verified a lot of it.

  1. There is a new Module called DMF_VirtualHidDeviceMini which is in the Library as a sibling of the original DMF_VirtualHidDeviceVhf. This is the root of the VHIDMINI2 sample.

  2. There is a new Module called DMF_VirtualHidDeviceMiniSample in the Template library. This Module is the Parent Module which contains only the HID device specific logic including descriptors. For your project, you should copy the DMF_VirtualHidDeviceMiniSample.c and .h files and create files appropriate for your device.(DMF_VirtualHidDeviceMiniSample has a Child Module, DMF_VirtualHidDeviceMini.)

  3. There are two new Sample Drivers. A Kernel Mode driver and (finally) a User-mode driver. Both these new samples do nothing more than instantiate a DMF_VirtualHidDeviceMiniSample and cause the HID devices to appear in Device Manager.

  4. You can compile the original sample application an run it against both Kernel and User-mode drivers.

Note the layering:

a. Sample drivers are just containers for DMF_VirtualHidDeviceMiniSample.
b. DMF_VirtualHidDeviceMiniSample contains only the logic for the original sample device, but no generic HID streaming code.
c. Finally, DMF_VirtualHidDeviceMini contains only the logic needed for HID streaming code and abstracts the differences between Kernel and User-mode for the Client (Module and Driver).

I will continue working on this branch until I am satisfied that everything is working and the quality of the code is good enough to check in to master and, obviously, it has been code reviewed (following our normal procedures).

However, you can use this branch to start working on your driver and see how it goes. If you see things that are obviously wrong, please create a pull request against that branch.

I hope this conversion shows you and others the differences between DMF and non-DMF drivers and how to convert non-DMF drivers to DMF drivers.

I am sorry for the delay in getting this done...It is not due to negligence...just conflicting priorities. Thank you for your patience. I am interested to get feedback from you. Like i said, I will concentrate on making sure the application works as it is supposed to.

@hansmbakker
Copy link
Author

Hi Sam,
wonderful, I'll check it out tonight when I am home! I really appreciate the effort you've done!

I'll let you know my feedback when I've tried it :)

@hansmbakker
Copy link
Author

Hi Sam,
I see you've already done a lot! Please find some first thoughts in #73.

@samtertzakian
Copy link
Member

Hi, Hans,
I agree with your feedback. I will try to make an update to address your feedback and correct the remaining issues I know about within a day or so.

@samtertzakian
Copy link
Member

Hi, Hans,

I have pushed a commit that shows how I prefer to make the HID definitions available to all Clients.
One thing about the Dmf_Xxx.h files is that they are very specific: They each reference a single Module and have a specific format. They are not designed to include general definitions. The change I made is very small and accomplishes the goal of making the definitions available to all Clients. If you have reserveration about this, let me know.

I have not been able to make the other changes yet but this was simple so I just got that in. I agree with your other feedback, however. I will not be able to get to it until next week.

@hansmbakker
Copy link
Author

hansmbakker commented Jul 25, 2019

Hi Sam,
I would take the definitions out of the VHF header file and put them in a separate file instead (as I did), because it feels counter-intuitive that one would have to reference the VHF header only to get the HID definitions, even when not using the VHF module at all (using the HID mini module instead).

I’ll wait for your update then, and I am curious how you think the timer code can best be restructured.

@samtertzakian
Copy link
Member

Hi Hans
I will try to make another update tomorrow or by end of week. I agree with your comments. Again, I apologize for the delay.

@hansmbakker
Copy link
Author

hansmbakker commented Aug 18, 2019

Hi Sam,
I saw you've been working on it in your branch, nice! (e.g. that you made the polling interval a module config property).

From a functional perspective I'm not sure whether the timer code should be in the HID module or in the driver that calls the HID module? (see my feedback in #73).

I noticed when manually queuing HID reports using the function in #73, they were correctly processed immediately (already before the timer expiry function was called).

Is that timer expiry function a kind of "cleanup" function? Or is it merely to simulate incoming HID reports?

If the latter is the case, then I believe the timer logic should be moved from https://github.com/microsoft/DMF/blob/personal/satertza/vhidmini2/Dmf/Modules.Library/Dmf_VirtualHidDeviceMini.c to https://github.com/microsoft/DMF/blob/personal/satertza/vhidmini2/Dmf/Modules.Template/Dmf_VirtualHidDeviceMiniSample.c

I would then keep a function like GenerateInputReport (from #73) in https://github.com/microsoft/DMF/blob/personal/satertza/vhidmini2/Dmf/Modules.Library/Dmf_VirtualHidDeviceMini.c and I would put the timer logic (which calls GenerateInputReport) in https://github.com/microsoft/DMF/blob/personal/satertza/vhidmini2/Dmf/Modules.Template/Dmf_VirtualHidDeviceMiniSample.c.

@samtertzakian
Copy link
Member

Hi, Hans,

I am working on a merge of latest updates to DMF. After that, I will continue to work on the VHIDMINI Module. When, I do so, I will look at your comments and try to address them one way or another. I hope to get back to this today or tomorrow.

One more thing: It so happens we are also going to be using the VHIDMINI Module internally so it we can get more feedback as well as actual use here. It should help to resolve many issues you would have encountered. So far the work has been geared toward creating parity with the existing sample.

Also, it seems I did not inform your that I had made changes. I don't see the comment on this thread. I thought I had. I will make sure to keep you up to date.

Thank you again for your feedback. I will address all your comments and queries above in a separate post when I have more time to consider all your points carefully.

@hansmbakker
Copy link
Author

That’s great to hear! I think more use benefits all :)

If there is something I can help with, let me know.

@samtertzakian
Copy link
Member

Hello, Hans.

I have good news for you. I have just submitted several changes to the branch to correct the bugs in the code. There are no known bugs at this time. The sample executable that tests the driver appears to be working properly. Also, other people here will begin using DMF_VirtualHidMini so we will get more feedback from them.

NOTE: The name of the Modules has changes to VirtualHidMini from VirtualHidDeviceMini per feedback here.

The code will go into master via a merge later next week.

Now, let me answer your questions from above:

You will see in the latest branch that I have made the changes you suggested as I agree with your assessment of how the input report should be handled (by the Client Module). My implementation is slightly different than yours, but it is the same idea. Give me your feedback about it. I will also get feed back here when people use it.

Thank you again for your time and feedback. I intended to respond more promptly but I was unable to due to various issues.

@hansmbakker
Copy link
Author

hansmbakker commented Aug 22, 2019

Hi Sam!

Nice! I see you’ve done a lot already! Thank you for the nice collaboration so far :)

I’m not able to take an in-depth look at it today, probably I can do that tomorrow or during the weekend.

@samtertzakian
Copy link
Member

Hello, Hans,
Sure, take your time. Thank you again. We really appreciate your feedback and patience.

@hansmbakker
Copy link
Author

hansmbakker commented Aug 23, 2019

Hi Sam,
I took a look at it and overall I think you've done a really good job!

My points of discussion would be the following:

  • it would be nice if the HID definitions from Dmf_VirtualHidDeviceVhf.h could be factored out into a common file (reasoning here), or was that a deliberate choice?
  • would it be possible to get the APIs of Dmf_VirtualHidDeviceVhf and Dmf_VirtualHidMini more similar?
  • could you explain why Dmf_VirtualHidDeviceVhf (and Dmf_VirtualHidKeyboard which uses it), just use a direct function call (DMF_VirtualHidDeviceVhf_ReadReportSend) while Dmf_VirtualHidMini uses a callback pattern for Input reports? Being able to call something like GenerateInputReport / DMF_VirtualHidDeviceVhf_ReadReportSend directly instead of using a callback system could result in less "ties" between the modules, and might make keeping the overview simpler.

I'm curious to hear your thoughts!

I had some guesses why a callback might be needed:

  • maybe the Dmf_VirtualHidMini needs to be able to read the current hardware status, like whether a button is pressed, at any time so there should be a function that the Dmf_VirtualHidMini can call any time?
  • maybe there is a need for delayed handling? I saw DMF_VirtualHidDeviceVhf_AsynchronousOperationComplete, but I was not sure in what case to use it and in what case to use it not (Dmf_HidPortableDeviceButtons uses it while Dmf_VirtualHidKeyboard module does not, while they are similar device types I would say?).

Is this something you could explain more? That way it might be also a point for me to learn.

@samtertzakian
Copy link
Member

samtertzakian commented Aug 26, 2019

Hi, Hans. I was travelling the past 3 days and unable to respond. I will try to answer all your questions later today. They are good questions that I hope will help others understand how DMF works.

@hansmbakker
Copy link
Author

hansmbakker commented Aug 26, 2019

That’s fine, I hope you had a nice trip :)

@hansmbakker
Copy link
Author

Hi Sam,
would you have time to look at my questions this week?

@samtertzakian
Copy link
Member

Hi, Hans, I apologize again for the delay. I have answered your questions here:

It would be nice if the HID definitions from Dmf_VirtualHidDeviceVhf.h could be factored out into a common file (reasoning here), or was that a deliberate choice?

Yes, I agree with you that it would be nice to place these definitions in a separate location since they are shared by a subset of Modules. But for right now, lets leave these definitions where they are. We have two classes of definitions right now: 1) Definitions shared by all Modules and 2) Definitions shared by a single Module. There is a place for both those classes. However, in the HID case, it is a different class where the definitions are shared by a subset of Modules (more than one Module). Currently there is no location for such definitions. I will discuss this issue and see if we can find a better way to expose such definitions. I agree with you in principle that right now the solution is not
optimal. Long term it is anticipated that VHF will be used for both Kernel and User-mode but I don't know when that will happen (or even if will happen). In that case, the definitions are already in a good location.

Would it be possible to get the APIs of Dmf_VirtualHidDeviceVhf and Dmf_VirtualHidMini more similar?

At the core VHF and VHIDMINI are different. VHF does a lot more and is actually a separate driver. The APIs exposed by DMF are based on the APIs exposed by the core (VHF or VHIDIMIN). Keep in mind that the new Module (DMF_VirtualHidMini) is designed to be a used for User-mode
drivers until support for User-mode in VHF is released. In that case, (again as above), it is anticipated that people would just use DMF_VirtualHidDeviceVhf and DMF_VirtualHidMini would be deprecated. Because the APIs are similar it would not be difficult to convert code that uses DMF_VirtualHidMini to use DMF_VirtualHidDeviceVhf.

Could you explain why Dmf_VirtualHidDeviceVhf (and Dmf_VirtualHidKeyboard which uses it), just use a direct function call (DMF_VirtualHidDeviceVhf_ReadReportSend) while Dmf_VirtualHidMini uses a callback pattern for Input reports?

VHF is much more robust than that VHIDMINI2. VHF performs its own queuing where as VHIDMINI has a simplified queuing system. The purpose of DMF_VirtualHidMini is to simply replicate the functionality of the MSDN VHIDHIMI2 sample so that people can easily used that sample code
to write drivers without copying all the underlying code that is not driver specific. This is why the APIs between the two Modules are different (related to the above answer). DMF_VirtualHidDeviceVhf_ReadReportSend relies heavily on the VHF API, VhfReadReportSubmit, which is not present in VHIDMINI2. It is the intention of DMF_VirtualHidMini to simply expose and simplify the use of VHIDMINI. It is not designed to replace VHF for User-mode especially since Andrew indicated that such work is in progress.

Being able to call something like GenerateInputReport / DMF_VirtualHidDeviceVhf_ReadReportSend directly instead of using a callback system could result in less "ties" between the Modules and might make keeping the overview simpler.

**I hope the above answers answer this question.

Having said all of the above: The nice thing about DMF is that if you want to modify the interface (as you indicated in your last point) you can do any of the following:

  1. You can add Methods to DMF_VirtualHidMini or modify existing Methods. We do this often by creating "Ex" versions of Methods so that the original Method is unchanged but programmers can used the improved Ex version for new code.

  2. You can use DMF_VirtualHiMini as a starting point (or example) to create your own Module with an interface you prefer.

The short answer to all your questions, it seems, is: DMF_VirtualHidMini is designed as a temporary measure until Microsoft fully unifies VHF for Kernel and User-mode. When that happens this new Module will become deprecated. I am awaiting feedback from others who are using
this new Module internally. If there are similar issues raised we can revisit.**

@hansmbakker
Copy link
Author

hansmbakker commented Aug 30, 2019

Hi Sam,
thank you for your explanations. I am wondering what feedback the other users will give.

The only thing I do not understand yet is the use of the DMF_VirtualHidDeviceVhf_AsynchronousOperationComplete method: Dmf_HidPortableDeviceButtons uses it while Dmf_VirtualHidKeyboard module does not, while they are similar device types I would say. Could you explain the difference between these two DMF modules?

(maybe this is out of scope for this issue but it made me wonder how these modules work)

@samtertzakian
Copy link
Member

Hello, Hans,
Sorry, I have again been dealing with high priority issues. Let me address your questions:

  1. A team is actively testing VirtualHidMini now. Because it is still being evaluated it is not included in today's merge. We only merge Modules once they are actually used in at least one production driver.

  2. DMF_VirtualHidDeviceVhf_AsynchronousOperationComplete() is used by a Client to complete a VHF request received by a callback function from VHF. When the Client receives a callback corresponding to a HID class IOCTL, the Client performs operations to satisfy the request. When the Client has completed the operations, the Client uses DMF_VirtualHidDeviceVhf_AsynchronousOperationComplete() to tell VHF can return the information to up the stack. This function must be used to tell VHF that the Client wants to complete a request initiated by VHF. Note the use of VhfOperationHandle which is passed by VHF.

The reason that the DMF_VirtualHidKeyboard does not use that call is that it does not support any calls initiated by VHF. In this Module the calls are initiated by the Client. Note that in this case there is no VhfOperationHandle . Instead, the Module calls DMF_VirtualHidDeviceVhf_ReadReportSend() to complete the pending read report sent by the HID keyboard driver previously. The Client that instantiates the DMF_VirtualHidKeyboard Module calls a Module Method when it wants to send a keystroke.

Let me know if that answers your question. I am happy to elaborate.

@hansmbakker
Copy link
Author

hansmbakker commented Sep 20, 2019

Sounds good 👍 thank you for the explanation! Also good to hear that a team is testing it.

Could you please also say something about #67? If the outcome is, that publishing this as a NuGet package is unfeasible, then that could also be an answer (so that it is clear for everyone).

Have a good weekend!

@samtertzakian
Copy link
Member

Hi, Hans,
I think in the comments above you mentioned that the single callback for read reports is not flexible. And based on feedback on this end, I see that the interface is not quite correct as you stated. I will be updating the branch with a correction for that in a couple of days. The change will be simple, in case you are already using it. Note that this Module has not yet been put in master branch. We will not do so until we have used it in a driver on our end. We are still working on that.

@hansmbakker
Copy link
Author

Ok, I'm curious how you will change it in that case!
Thank you for your ongoing work on this!

@samtertzakian
Copy link
Member

Hello, Hans,
Just to give you an update. We are still working on formal version of DMF_VirtualHidMini Module. We are using it in a driver to help us flush out any issues. My feeling is we will submit the formal version late next week. It is not substantially changed from what is in the branch now, but there are some changes.

@hansmbakker
Copy link
Author

Hi Sam,
could you share any news on this?

@samtertzakian
Copy link
Member

Hi, Hans,
We have been using the a Module similar to what is the branch now. We will try to make an update this week in master branch along with the example.

@samtertzakian
Copy link
Member

samtertzakian commented Dec 9, 2019

Hi, Hans, we have not forgotten about you. We wanted to debug one issue...we are making a merge now...I hope to merge in the update today or tomorrow. Note that the Module is similar to what is in the branch I sent before.

@samtertzakian
Copy link
Member

Hi, Hans, the final PR for VirtualHidMini into master is pending now. I am waiting for one person to approve it...I think by tomorrow it will be in. Thank you for your patience. That code is now being used in a production driver so it does have some milage.

@samtertzakian
Copy link
Member

Hi, Hans.... the DMF_VirtualHidMini and associated sample is in master, finally. Thank you for your patience. Let me know if you have any questions.

@samtertzakian
Copy link
Member

Hi, Hans,
I thought the Sample driver was merged but it was not. I am doing that now. I will make one more update to master and a new release to fix that.

@samtertzakian
Copy link
Member

Hi, Hans,

Ok, the sample is there. There was just a mistake in the project file for the library path. I am making a PR for that now.

@samtertzakian
Copy link
Member

Hi, Hans,
LKG 1.1.34 has the fix for project file. I will close this issue.

@hansmbakker
Copy link
Author

Great @samtertzakian, thank you very much for this! I will certainly include this in my project!

@samtertzakian
Copy link
Member

Great, please refer to the sample:
https://github.com/microsoft/DMF/tree/master/DmfSamples/VHidMini2Dmf
Contact me if you have questions.

@feature-engineer
Copy link

I see that vhf can now be used in user mode as well as kernel mode - so this sample is deprecated, right? Where can I find sample code for user mode vhf based driver?

@samtertzakian
Copy link
Member

samtertzakian commented May 21, 2023 via email

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

No branches or pull requests

4 participants