sensor: Introduce Phosense XBR818 Driver #81013
Conversation
There was a problem hiding this comment.
add an entry to the build_all test
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/build_all/sensor/i2c.dtsi
Addressed all the comments, can you confirm what i've added to i2c.dtsi is correct? Thanks. Edit: I'm not sure what is causing the build failure, the changes I made shouldn't affect this, unless unknown vendor prefix breaks the test (which it shouldnt do knowing the majority of vendors are not in that list and i was told it's only a copy of the linux one, so we cant add to it). |
Try adding the vendor name over here |
Done, but I am concerned it isn't fine, though maybe I misunderstand something. |
|
check error now is relevant to additional sensor attributes, not sure what to do given this is a pattern that exists in other drivers, maybe I am not doing it how it's supposed to be? |
|
give it a try with parenthesis
|
2d3bcad to
13b3571
Compare
I dont think so, looks to be only if there is more than one sensor of the vendor (eg sensor/bosch folder vs sensor/mhz19b or sensor/hp206c for example) |
|
Found a couple more cases of variable possibly spending longer time on stack than needed so i removed the init at 0 for them too. |
|
ended up using a workaround to remove the enum thing, it should now pass the tests. |
|
|
||
| i2c-en-gpios: | ||
| type: phandle-array | ||
| required: true |
There was a problem hiding this comment.
Making this required might be overly restrictive. I believe it's quite common that people who might not care all that much about e.g. power consumption would wire things so that I2C is always enabled. Code would need updating so that only when pin has been configured it's being toggled, otherwise assume always on. See e.g. https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/haptics/drv2605.c#L550-L559
That being said, I'm counting on other sensor collaborators to confirm whether what I'm saying makes sense :)
There was a problem hiding this comment.
It makes sense, I didnt consider the possibility of always on, only always off where my thought was 'no driver needed in that case'.
drivers/sensor/xbr818/xbr818.c
Outdated
| ret = gpio_pin_set_dt(&config->i2c_en, 1); | ||
| if (ret != 0) { | ||
| LOG_ERR("%s: could not set i2c_en pin", dev->name); | ||
| } |
There was a problem hiding this comment.
you may want to extract all these i2c-enable / i2c-disable in an (inline?) function to avoid duplication
what's more, looking at what seems to be some kind of reference implementation (given the datasheet is really not that helpful...), I wonder if you want to add 10us delay to be on the safe side? https://github.com/Ai-Thinker-Open/STM32F102_Rd-04/blob/master/Rd-04/rd_04_driver.c#L109-L110
There was a problem hiding this comment.
answering to your question mark with this from the Contribution guidelines :)
Best way is to basically leave the conversation "unresolved" so that the person who request/suggested the change can go back and provide further feedback / eventually resolve the comment. This is also a good way for reviewer to not have to keep mental notes of all the things they may have asked in the past and that they need to go and double check, as it's really hard to do once all conversations have already been resolved and are collasped in the Github UI
There was a problem hiding this comment.
so... unresolving until I have another look :) and even if the conversation doesn't get "resolved" in Github, that deosn't prevent reviewer to eventually give a +1 anyway
There was a problem hiding this comment.
Alright, I'll leave the conversations open in the future.
drivers/sensor/xbr818/xbr818.h
Outdated
| #define SENSOR_ATTR_XBR818_DELAY_TIME (SENSOR_ATTR_PRIV_START) | ||
| #define SENSOR_ATTR_XBR818_LOCK_TIME (SENSOR_ATTR_PRIV_START + 1) | ||
| #define SENSOR_ATTR_XBR818_NOISE_FLOOR (SENSOR_ATTR_PRIV_START + 2) | ||
| #define SENSOR_ATTR_XBR818_RF_POWER (SENSOR_ATTR_PRIV_START + 3) |
There was a problem hiding this comment.
Those need to be in a public header file, which this file isn't, if you want people to be able to use this :) i.e. they need to be moved to a include/zephyr/drivers/sensor/xbr818.h header.
Also please make sure this has proper Doxygen comments. See e.g. https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/sensor/tmag5273.h#L36-L49
There was a problem hiding this comment.
Also please make sure this has proper Doxygen comments.
Can you please take another pass, as Doxygen comments are missing (see, a good reason for not closing conversations after all 😉 ).
https://builds.zephyrproject.io/zephyr/pr/81013/docs/doxygen/html/xbr818_8h.html#a2f524848967d71fe6c7557355a064bec
you want something like tmag5273.h that I pointed you to, where the enum and all its fields have proper doxygen comments. (
There was a problem hiding this comment.
Then I have not properly identified what constitute a proper doxygen comment, what is it? the /** instead of /* at the start?
There was a problem hiding this comment.
Ok, figured it out.
3fbb4f5 to
469ae64
Compare
|
Rebased on latest main. |
This Introduces a driver for the i2c interface of Phosense XBR818. XBR818 is a 10.525Ghz Radar chip with builtin detection algorithm. Signed-off-by: Camille BAUD <mail@massdriver.space>
This Introduces a driver for the i2c interface of Phosense XBR818.
XBR818 is a 10.525Ghz Radar chip with in-built movement detection algorithm.