Skip to content

add Si12T touch driver#1607

Closed
stc1988 wants to merge 2 commits into
Moddable-OpenSource:publicfrom
stc1988:dirver/si12T
Closed

add Si12T touch driver#1607
stc1988 wants to merge 2 commits into
Moddable-OpenSource:publicfrom
stc1988:dirver/si12T

Conversation

@stc1988
Copy link
Copy Markdown
Contributor

@stc1988 stc1988 commented May 9, 2026

This driver is used to pet the M5 StackChan.  I don’t plan to add the StackChan device itself, but this driver might be needed elsewhere, so I created a PR.

datasheet

@stc1988 stc1988 marked this pull request as ready for review May 9, 2026 05:53
Copy link
Copy Markdown
Collaborator

@phoddie phoddie left a comment

Choose a reason for hiding this comment

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

Generally this looks good. It kind of feels like it was translated from C? There are a lot of single use private methods. I marked one place that happens, but there are more. II suggest eliminating those in most cases:

  • There is some call overhead
  • There is some code overhead
  • There is RAM overhead – because each private method uses a RAM slot as JavaScript requires them to be implemented as private fields.

Comment thread modules/drivers/sensors/si12t/si12t.js Outdated
Comment thread modules/drivers/sensors/si12t/si12t.js Outdated
Comment thread modules/drivers/sensors/si12t/si12t.js Outdated
Comment thread modules/drivers/sensors/si12t/si12t.js Outdated
Comment thread modules/drivers/sensors/si12t/si12t.js
@stc1988
Copy link
Copy Markdown
Contributor Author

stc1988 commented May 10, 2026

Generally this looks good. It kind of feels like it was translated from C? There are a lot of single use private methods. I marked one place that happens, but there are more. II suggest eliminating those in most cases:

There is some call overhead
There is some code overhead
There is RAM overhead – because each private method uses a RAM slot as JavaScript requires them to be implemented as private fields.

Thanks for the review. You’re right—I did refer to a C implementation, but I also adjusted some parts, like making certain logic more explicit so it works better as comments for Moddable. Still, it looks like there’s more I need to improve, so I’ll take another look.

@phoddie
Copy link
Copy Markdown
Collaborator

phoddie commented May 10, 2026

Thanks for being open to taking another look. I find that small, relatively simple projects like this one are a good opportunity to refine how I organize code.

...like making certain logic more explicit so it works better as comments for Moddable...

Interesting. I sensed that the helper function names were kind of serving the purpose of comments. As many people have observed, I'm not a big user of actual comments in code, but they do have zero runtime cost. ;)

@stc1988
Copy link
Copy Markdown
Contributor Author

stc1988 commented May 10, 2026

I reviewed and removed unnecessary private functions across the entire codebase, not just in the areas that were pointed out. Of course, I’ll also review everything myself, but I’ll incorporate these review learnings into the AI as well.

@phoddie
Copy link
Copy Markdown
Collaborator

phoddie commented May 10, 2026

Thank you for the changes. This is more straightforward to read and has a lighter runtime impact too. Best of both worlds.

@mkellner
Copy link
Copy Markdown
Collaborator

This has been merged and will be available in the next code drop.

@mkellner mkellner closed this May 11, 2026
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.

3 participants