Skip to content

Conversation

@ecpeterson
Copy link
Contributor

@ecpeterson ecpeterson commented Dec 31, 2024

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:

  1. mtds_hello: Writes "Hello, World!" in green text at the top-left corner of the screen. Based off of MtdsDemo1.pde:MtdsTest21.
  2. 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:

1> pmod_mtds:start_link(spi2).
{ok,<0.314.0>}
2> mtds_sketch:start_link().
{ok,<0.317.0>}
% scribbled 'GRiSP' with my finger
3> gen_server:stop(pid(0,317,0)).
ok
% needed to release the display handle
4> mtds_hello:run().
ok
% printed 'Hello, World!' in green text in the corner
5> mtds_sketch:start_link().
{ok,<0.321.0>}
% back to scribbling

NOTE: I used OTP 27's -doc directives. 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.

Copy link
Member

@bchassoul bchassoul left a 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

@ecpeterson
Copy link
Contributor Author

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!

Copy link
Member

@GwendalLaurent GwendalLaurent left a 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

Comment on lines 265 to 271
% 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}.
Copy link
Member

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 ?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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!)

@ecpeterson
Copy link
Contributor Author

Thanks for the review. I made the more trivial PR changes; I'll follow up with the others later this weekend.

@ecpeterson
Copy link
Contributor Author

Alright: ready for re-review, I think.

@GwendalLaurent GwendalLaurent self-requested a review January 17, 2025 15:07
@ecpeterson ecpeterson requested a review from bchassoul January 22, 2025 14:13
@odo
Copy link
Contributor

odo commented Jan 31, 2025

Hey @bchassoul, any chance of a review? Would love to use a touch display on my GRiSP board 8)

@GwendalLaurent GwendalLaurent merged commit 89219d9 into grisp:master Feb 3, 2025
5 checks passed
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