| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 42
MTDS driver #151
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
MTDS driver #151
Conversation
bchassoul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Eric, nice PR! 👍
My comments are just small details, we use Edoc for documentation across the codebase, so would you mind updating the documentation to follow the Edoc style for consistency?
I haven’t tested it with the Pmod, but from reviewing the code, I couldn’t find any additional issue
|
I've never edoc-ed before, so I've only done my best to mimic what I saw elsewhere. Hope this satisfies. Thanks for the review! |
GwendalLaurent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, thank you for your contribution this is a very nice PR 👍
I was able to test the 2 examples and they were working as expected.
Additionally to the comments I already left in the code, I have a more general comment concerning the definitions of the types and the macros. I think it's better for readability if they are all defined at the top of the file (especially for the types). For the macros, if there are too many of them they can also be moved to a hrl file.
Your changes to edoc looks great. However, you forgot to do the change it in the 2 examples as well
src/pmod_mtds.erl
Outdated
| % Initializes the bus object to form a driver state, but defers actual device | ||
| % initialization to a timeout (see handle_info/2) so as not to get in the way of | ||
| % any supervisor tree. | ||
| init([Interface]) -> | ||
| % NOTE: MTDS wants to put freq in 3.5–4 MHz, whereas GRiSP runs at 0.1 MHz. | ||
| ok = grisp_devices:register(Interface, ?MODULE), | ||
| {ok, Interface, 0}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the initialization of the driver is splitted in 2 parts. Could you explain your reasoning behind this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more habit than really careful reasoning: blocking calls in init also block any parent supervision tree from making progress with the rest of its own boot, so I was taught to move those out of the way. I don't really know how reliably / quickly the MTDS comes up, so I followed the same pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point of view. However, in our case, we want to add the the driver to the device supervision tree only after a full and correct initialization of the pmod and its driver. I would move the initialization instruction done after the timeout back into the init/1 function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced, but I agree that this is beyond the scope of this PR and I should just do whatever the other device drivers do. I've moved the whole of bring-up into init/1. (I also just learned about handle_continue/2, which is better than the outdated abuse of timeouts!)
|
Thanks for the review. I made the more trivial PR changes; I'll follow up with the others later this weekend. |
|
Alright: ready for re-review, I think. |
|
Hey @bchassoul, any chance of a review? Would love to use a touch display on my GRiSP board 8) |
Here's a driver for the MTDS module. It implements only a small subset of the C++ MTDS driver's functionalities, but it's a representative subset: I think any other desired feature could be implemented by finding where the C++ driver constructs the relevant payloads and copy-pasting the routines I've written with the protocol constants changed. Functionality which is covered includes relaying touch events to interested Erlang-side applications, drawing lines, printing text, setting color options, and double-buffering (although I did not test this last one).
I also included two demos:
mtds_hello: Writes "Hello, World!" in green text at the top-left corner of the screen. Based off ofMtdsDemo1.pde:MtdsTest21.mtds_sketch: An Erlang server which listens for touch events and converts them into draw events, so you can draw on the screen with your finger.Sample run of the two included demos:
NOTE: I used OTP 27's
-docdirectives. I'm happy to change them to edoc comments if that's unacceptable — though it will take me a minute, since I've never used edoc comments before.