Skip to content

Conversation

it-klinger
Copy link
Contributor

  • add driver for Vishay VEML6046 RGBIR color sensor
  • add new compatible "vishay,veml6046"
  • support fetch and get sensor subsystem operations
  • triggered mode and interrupts are not yet supported
  • add attribution test application which tests all attribute combinations

uint32_t green_lux;
uint32_t blue_lux;
uint32_t ir_lux;
uint32_t int_flags;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused. I'll remove.

Comment on lines 401 to 367
case SENSOR_ATTR_LOWER_THRESH:
val->val1 = data->thresh_low;
break;
case SENSOR_ATTR_UPPER_THRESH:
val->val1 = data->thresh_high;
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

should convert back to lux

return ret;
}

ret = veml6046_read16(dev, VEML6046_CMDCODE_INT_L, val);
Copy link
Contributor

Choose a reason for hiding this comment

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

may want to actually check ret?

sensor_channel_get(dev, SENSOR_CHAN_RED, &red);
sensor_channel_get(dev, SENSOR_CHAN_VEML6046_RED_RAW_COUNTS, &red_raw);

sensor_channel_get(dev, SENSOR_CHAN_RED, &green);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sensor_channel_get(dev, SENSOR_CHAN_RED, &green);
sensor_channel_get(dev, SENSOR_CHAN_GREEN, &green);

sensor_channel_get(dev, SENSOR_CHAN_RED, &green);
sensor_channel_get(dev, SENSOR_CHAN_VEML6046_GREEN_RAW_COUNTS, &green_raw);

sensor_channel_get(dev, SENSOR_CHAN_RED, &blue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sensor_channel_get(dev, SENSOR_CHAN_RED, &blue);
sensor_channel_get(dev, SENSOR_CHAN_BLUE, &blue);

sensor_channel_get(dev, SENSOR_CHAN_RED, &blue);
sensor_channel_get(dev, SENSOR_CHAN_VEML6046_BLUE_RAW_COUNTS, &blue_raw);

sensor_channel_get(dev, SENSOR_CHAN_RED, &ir);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sensor_channel_get(dev, SENSOR_CHAN_RED, &ir);
sensor_channel_get(dev, SENSOR_CHAN_IR, &ir);

@it-klinger it-klinger force-pushed the add-veml6046 branch 4 times, most recently from 565677d to d7051b9 Compare May 26, 2025 13:18
Copy link

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 26, 2025
@github-actions github-actions bot closed this Aug 10, 2025
@it-klinger
Copy link
Contributor Author

Hi,
can anybody review the patch, please?
I think i changed everything according to the review or did i miss something?

@kartben
Copy link
Contributor

kartben commented Aug 10, 2025

Hi,
can anybody review the patch, please?
I think i changed everything according to the review or did i miss something?

It will be possible as soon as you restore https://github.com/it-klinger/zephyr/tree/add-veml6046 to commit d7051b9 which is what it was when the PR was closed. Hope this helps!

@it-klinger
Copy link
Contributor Author

Hi Benjamin,
i restored to d7051b9 and also did a small change and force commited, but the PR is not opening again.

@kartben
Copy link
Contributor

kartben commented Aug 10, 2025

Hi Benjamin,
i restored to d7051b9 and also did a small change and force commited, but the PR is not opening again.

It's only when the branch is exactly at that commit that reopening will be possible. The follow-up commit will have to be done afterwards

@kartben
Copy link
Contributor

kartben commented Aug 10, 2025

Hi Benjamin,
i restored to d7051b9 and also did a small change and force commited, but the PR is not opening again.

It's only when the branch is exactly at that commit that reopening will be possible. The follow-up commit will have to be done afterwards

And right now neither of the two commits are d705 anyway main...it-klinger:zephyr:add-veml6046

@it-klinger
Copy link
Contributor Author

it-klinger commented Aug 10, 2025

Now it's at d705 but i'm only able to open a PR with a new number (#94305), but the closed one is not opened.

@kartben kartben reopened this Aug 10, 2025
josuah
josuah previously approved these changes Oct 9, 2025
Copy link
Contributor

@josuah josuah left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the modifications!

MaureenHelm
MaureenHelm previously approved these changes Oct 17, 2025
JarmouniA
JarmouniA previously approved these changes Oct 17, 2025
@JarmouniA JarmouniA added this to the v4.3.0 milestone Oct 17, 2025
@kartben kartben requested a review from Copilot October 17, 2025 19:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a new Zephyr driver for the Vishay VEML6046 RGBIR sensor, introduces a shared VEML60xx common header for family-wide enums/utilities, and provides a sample plus DTS binding. It also refactors the existing VEML6031 code to reuse the shared definitions.

  • New VEML6046 driver with fetch/get/attr support and build integration
  • New common VEML60xx header (integration time, gain, persistence enums/utilities) and VEML6031 refactor
  • New sample app, DTS binding, and test/build wiring

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
tests/drivers/build_all/sensor/i2c.dtsi Adds a VEML6046 I2C node to exercise build-all for the new driver.
samples/sensor/veml6046/src/main.c New sample that sweeps attributes and reads RGBIR and raw counts.
samples/sensor/veml6046/sample.yaml Test harness config for the VEML6046 sample.
samples/sensor/veml6046/prj.conf Enables SENSOR for the sample.
samples/sensor/veml6046/boards/olimex_stm32_e407.overlay Board overlay wiring VEML6046 on I2C2 with alias rgbir.
samples/sensor/veml6046/README.rst Sample documentation and example output.
samples/sensor/veml6046/CMakeLists.txt CMake for the sample.
samples/sensor/veml6031/src/main.c Switches to shared VEML60xx enums for IT/gain.
include/zephyr/drivers/sensor/veml60xx-common.h New common header for VEML60xx family enums, timing table, and range checks.
include/zephyr/drivers/sensor/veml6046.h New VEML6046-specific attributes and private channels definitions.
include/zephyr/drivers/sensor/veml6031.h Uses the new common header; removes duplicated enums.
dts/bindings/sensor/vishay,veml6046.yaml New DT binding for VEML6046.
drivers/sensor/vishay/veml6046/veml6046.c New VEML6046 driver implementation.
drivers/sensor/vishay/veml6046/Kconfig Kconfig entry to enable the driver when DTS compatible is present.
drivers/sensor/vishay/veml6046/CMakeLists.txt Adds the driver source to the build.
drivers/sensor/vishay/veml6031/veml6031.c Refactors to use shared VEML60xx enums and timing table.
drivers/sensor/vishay/Kconfig Includes VEML6046 Kconfig in Vishay sensors menu.
drivers/sensor/vishay/CMakeLists.txt Adds VEML6046 subdirectory conditionally.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 504 to 518
struct veml6046_data *data = dev->data;

if (!data->sd) {
switch (action) {
case PM_DEVICE_ACTION_SUSPEND:
return veml6046_set_shutdown_flag(dev, 1);

case PM_DEVICE_ACTION_RESUME:
return veml6046_set_shutdown_flag(dev, 0);

default:
return -ENOTSUP;
}
}
return 0;
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

Resume will never clear shutdown after a prior suspend because the switch is gated by if (!data->sd). Remove the outer condition and handle SUSPEND/RESUME unconditionally so RESUME can bring the device back from sd=1.

Suggested change
struct veml6046_data *data = dev->data;
if (!data->sd) {
switch (action) {
case PM_DEVICE_ACTION_SUSPEND:
return veml6046_set_shutdown_flag(dev, 1);
case PM_DEVICE_ACTION_RESUME:
return veml6046_set_shutdown_flag(dev, 0);
default:
return -ENOTSUP;
}
}
return 0;
switch (action) {
case PM_DEVICE_ACTION_SUSPEND:
return veml6046_set_shutdown_flag(dev, 1);
case PM_DEVICE_ACTION_RESUME:
return veml6046_set_shutdown_flag(dev, 0);
default:
return -ENOTSUP;
}

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a valid comment to me, unless I am missing something

Comment on lines 99 to 109
inline bool veml60xx_gain_in_range(int32_t gain)
{
return (gain >= VEML60XX_GAIN_1) && (gain <= VEML60XX_GAIN_0_5);
}

inline bool veml60xx_it_in_range(int32_t it)
{
return (it >= VEML60XX_IT_3_125) && (it <= VEML60XX_IT_400);
}

inline bool veml60xx_pers_in_range(int32_t pers)
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

These functions are defined in a header with external linkage; this can lead to multiple-definition or missing-definition issues. Mark them static inline to give them internal linkage in each translation unit.

Suggested change
inline bool veml60xx_gain_in_range(int32_t gain)
{
return (gain >= VEML60XX_GAIN_1) && (gain <= VEML60XX_GAIN_0_5);
}
inline bool veml60xx_it_in_range(int32_t it)
{
return (it >= VEML60XX_IT_3_125) && (it <= VEML60XX_IT_400);
}
inline bool veml60xx_pers_in_range(int32_t pers)
static inline bool veml60xx_gain_in_range(int32_t gain)
{
return (gain >= VEML60XX_GAIN_1) && (gain <= VEML60XX_GAIN_0_5);
}
static inline bool veml60xx_it_in_range(int32_t it)
{
return (it >= VEML60XX_IT_3_125) && (it <= VEML60XX_IT_400);
}
static inline bool veml60xx_pers_in_range(int32_t pers)

Copilot uses AI. Check for mistakes.

enum veml6031_pers pers; /* ALS persistens protect */
enum veml60xx_gain gain; /* gain selection */
enum veml60xx_it itim; /* ALS integration time */
enum veml60xx_pers pers; /* ALS persistens protect */
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'persistens' to 'persistence'.

Suggested change
enum veml60xx_pers pers; /* ALS persistens protect */
enum veml60xx_pers pers; /* ALS persistence protect */

Copilot uses AI. Check for mistakes.

Comment on lines 56 to 59
Red: 68 lx ( 51) green: 68 lx ( 84) blue: 68 lx ( 51) IR: 68 lx ( 27) it: 0 div2: 0 gain: 0 --
Red: 121 lx ( 181) green: 121 lx ( 347) blue: 121 lx ( 240) IR: 121 lx ( 53) it: 0 div2: 0 gain: 1 --
Red: 215 lx ( 106) green: 215 lx ( 226) blue: 215 lx ( 160) IR: 215 lx ( 19) it: 0 div2: 0 gain: 2 --
Red: 201 lx ( 75) green: 201 lx ( 156) blue: 201 lx ( 112) IR: 201 lx ( 14) it: 0 div2: 0 gain: 3 --
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

Sample output labels 'div2', but the code and API use 'pdd' (effective photodiode size). Update the sample output to use 'pdd' for consistency.

Suggested change
Red: 68 lx ( 51) green: 68 lx ( 84) blue: 68 lx ( 51) IR: 68 lx ( 27) it: 0 div2: 0 gain: 0 --
Red: 121 lx ( 181) green: 121 lx ( 347) blue: 121 lx ( 240) IR: 121 lx ( 53) it: 0 div2: 0 gain: 1 --
Red: 215 lx ( 106) green: 215 lx ( 226) blue: 215 lx ( 160) IR: 215 lx ( 19) it: 0 div2: 0 gain: 2 --
Red: 201 lx ( 75) green: 201 lx ( 156) blue: 201 lx ( 112) IR: 201 lx ( 14) it: 0 div2: 0 gain: 3 --
Red: 68 lx ( 51) green: 68 lx ( 84) blue: 68 lx ( 51) IR: 68 lx ( 27) it: 0 pdd: 0 gain: 0 --
Red: 121 lx ( 181) green: 121 lx ( 347) blue: 121 lx ( 240) IR: 121 lx ( 53) it: 0 pdd: 0 gain: 1 --
Red: 215 lx ( 106) green: 215 lx ( 226) blue: 215 lx ( 160) IR: 215 lx ( 19) it: 0 pdd: 0 gain: 2 --
Red: 201 lx ( 75) green: 201 lx ( 156) blue: 201 lx ( 112) IR: 201 lx ( 14) it: 0 pdd: 0 gain: 3 --

Copilot uses AI. Check for mistakes.

# SPDX-License-Identifier: Apache-2.0

description: |
Vishay VEML6046 RGBIR color Sensor With I2C Interface.
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

Use sentence casing and correct capitalization: 'Vishay VEML6046 RGBIR color sensor with I2C interface.'

Suggested change
Vishay VEML6046 RGBIR color Sensor With I2C Interface.
Vishay VEML6046 RGBIR color sensor with I2C interface.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah sure, if you fix other spelling mistakes why not :)

#include <zephyr/kernel.h>

#include <zephyr/sys/printk.h>
#include <zephyr/sys_clock.h>
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

This header is not used in the sample; remove it to reduce unnecessary includes.

Suggested change
#include <zephyr/sys_clock.h>

Copilot uses AI. Check for mistakes.


#include <zephyr/device.h>
#include <zephyr/drivers/sensor.h>
#include <zephyr/drivers/i2c.h>
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

This include is unused in this sample; it can be removed.

Suggested change
#include <zephyr/drivers/i2c.h>

Copilot uses AI. Check for mistakes.

@kartben
Copy link
Contributor

kartben commented Oct 17, 2025

Would be nice to address the PM comment as well as static inline from copilot, the rest is probably secondary. You would need to please rebase on main in any case, and solve the merge conflict with tests/drivers/build_all/sensor/i2c.dtsi
Thank you!

@it-klinger
Copy link
Contributor Author

Thanks for the reviews and comments.
Implemented all comments and did a rebase to main.

@it-klinger
Copy link
Contributor Author

Reopen again.

@it-klinger it-klinger reopened this Oct 18, 2025
@it-klinger it-klinger dismissed stale reviews from JarmouniA, MaureenHelm, and josuah via 0f2518f October 18, 2025 08:12
@zephyrbot zephyrbot requested a review from MaureenHelm October 18, 2025 08:14
- add driver for Vishay VEML6046 RGBIR color sensor
- add new compatible "vishay,veml6046"
- support fetch and get sensor subsystem operations
- triggered mode and interrupts are not yet supported

Signed-off-by: Andreas Klinger <[email protected]>
- Test all attribute combinations of Vishay RGBIR color sensor VEML6046.
- Print OVERFLOW in case of saturation of sensor.
- This small program is intended to be helping when finding appropriate
  attributes for an application of the sensor.

Signed-off-by: Andreas Klinger <[email protected]>
- create common header file veml60xx-common.h for sensors VEML6031 and
  VEML6046.

Signed-off-by: Andreas Klinger <[email protected]>
Copy link

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copy link
Contributor

@josuah josuah left a comment

Choose a reason for hiding this comment

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

Thank you for the successive modifications and improvements!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards/SoCs area: Devicetree Bindings area: Samples Samples area: Sensors Sensors area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants