-
Notifications
You must be signed in to change notification settings - Fork 8.1k
drivers: sensor: microchip: mtch9010 Added MTCH9010 Device Support #91812
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
drivers: sensor: microchip: mtch9010 Added MTCH9010 Device Support #91812
Conversation
If possible, could we add this for v4.2 |
gpio_pin_set_dt(&config->enableUARTLine, GPIO_OUTPUT_LOW); | ||
} else { | ||
gpio_pin_set_dt(&config->enableUARTLine, GPIO_OUTPUT_HIGH); |
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.
copy-paste error from previous block
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.
Ah, I see. UART EN is active LOW. Clarified intention in comments
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.
no the issue is basically the same thing @msalau commented on. You want to be controlling enableCFGLine here, not enableUARTLine??
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.
Thanks - I think I see the issue now
tests/drivers/sensor/mtch9010/test_configs/data_format/current.overlay
Outdated
Show resolved
Hide resolved
cf24bb0
to
e4ac4a3
Compare
@kartben I'm looking at the twister failures, and it doesn't seem related to my PR, as I don't utilize with the file system. Can you confirm or rerun/ignore that twister error? |
yep, had nothing to do with you, and fixed now :) |
data->last_heartbeat = k_uptime_get(); | ||
|
||
/* Wait for boot-up */ | ||
k_msleep(CONFIG_MTCH9010_BOOT_TIME_MS); |
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.
Just highlighting that this will delay boot up process for all.
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.
Unfortunately, things stop working when I remove this delay. I have some theories, but for now, I'd like to keep this.
LOG_INST_DBG(config->log, "UART setup not enabled"); | ||
|
||
#ifdef CONFIG_MTCH9010_LOCK_ON_STARTUP | ||
BUILD_ASSERT(true, "MTCH9010_LOCK_ON_STARTUP cannot be set without UART enabled"); |
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.
Location of this BUILD_ASSERT()
is somewhat strange to me. It is not a part of the initialization routine. I think it should be closer to MTCH9010_DEFINE()
and be checked at build time.
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.
Moved assertion to define section 👍
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.
Please do not resolve comments. This is for the reviewers to do
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.
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.
My bad - thanks for letting me know
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.
The BUILD_ASSERT keeps causing issues, and since this is an optional feature anyway, I've removed the BUILD_ASSERT.
511c14e
to
3b46797
Compare
Looks like everything is good to go on our side 👍 Local test bench is functioning as expected |
I am marking it as tentative for 4.2 but my guess is that it will be tight as it is quite a significant PR and it will take time for reviewers to go through it. Thanks! |
Sonar Qube issue needs attention In general, you should not be validating the value of DT properties that have enum/range in binding, bcz it's done during DT parsing. |
1386716
to
b73a750
Compare
|
SonarQube now passing |
All of the requested changes have been made - can you review and approve, so we can close this PR? |
@msalau @kartben @MaureenHelm Can you re-review the PR for 4.3? |
Do I need to do anything else for it to be merged, or is it automatic? |
You're all set. It will be merged shortly :) |
I am triggering a fresh CI run since the last one was pretty old, just in case |
Looks like Twister build prep is failing - I don't think that's on my side? |
ah :| you actually need to rebase on main, please. It won't dismiss your existing approvals :) |
Added support for MTCH9010 and applied edits from reviewers. Signed-off-by: Robert Perkel <[email protected]>
Added testsuite to verify MTCH9010 sensor driver Signed-off-by: Robert Perkel <[email protected]>
b73a750
to
e5fd633
Compare
|
Added new driver for MTCH9010 liquid leak detector along with test patterns.