sensor: afbr-s50: Introduce 3D Time-of-Flight sensor

[ubieda]

Description

This PR introduces a 3D ToF sensor, including:

• Zephyr Sensor API v2 Support:
  • One-shot readings (Read/Decode).
  • Streaming mode (Data Ready triggers).
• Supported data-channels:
  • 1D Results SENSOR_CHAN_DISTANCE.
  • 3D results SENSOR_CHAN_AFBR_S50_PIXELS (32-pixel Matrix of distance measurements: 4 x 8).

Note

This PR includes an external module. Marking as DNM until reviewed and approved by the Zephyr Project TSC/Governing Board.

Testing

This PR has been tested with MCXN-947 Freedom EVK and AFBR-S50 Adapter board.

• DTS Overlay (frdm_mcxn947_mcxn947_cpu0.overlay):

#include <nxp/mcx/MCXN947VDF-pinctrl.h>

&pinctrl {
	pinmux_flexcomm1_lpspi: pinmux_flexcomm1_lpspi {
		group0 {
			pinmux = <FC1_P0_PIO0_24>,
				<FC1_P1_PIO0_25>,
				<FC1_P2_PIO0_26>,
				<FC1_P3_PIO0_27>;
			slew-rate = "fast";
			drive-strength = "high";
			input-enable;
		};
	};
	pinmux_gpio_lpspi: pinmux_gpio_lpspi {
		group0 {
			pinmux = <PIO0_24>,
				<PIO0_25>,
				<PIO0_26>,
				<PIO0_27>;
			slew-rate = "fast";
			drive-strength = "high";
		};
	};
};


#include <zephyr/dt-bindings/sensor/afbr_s50.h>

&flexcomm1_lpspi1 {
	status = "okay";
	pcs-sck-delay = <25>;
	sck-pcs-delay = <25>;
	transfer-delay = <0>;

	pinctrl-0 = <&pinmux_flexcomm1_lpspi>;
	pinctrl-1 = <&pinmux_gpio_lpspi>;
	pinctrl-names = "default", "priv_start";

	cs-gpios = <&gpio0 27 GPIO_ACTIVE_LOW>;

	afbr_s50: afbr_s50@0 {
		compatible = "brcm,afbr-s50";
		reg = <0>;
		spi-max-frequency = <2000000>;

		odr = <10>;
		dual-freq-mode = <AFBR_S50_DT_DFM_MODE_4X>;
		measurement-mode = <AFBR_S50_DT_MODE_LONG_RANGE_LASER_CLASS_2>;

		int-gpios = <&gpio0 10 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
		spi-mosi-gpios = <&gpio0 24 GPIO_ACTIVE_HIGH>;
		spi-sck-gpios = <&gpio0 25 GPIO_ACTIVE_HIGH>;
		spi-miso-gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>;
	};
};

• Build Command:

west build -b frdm_mcxn947/mcxn947/cpu0 samples/hello_world -- -DCONFIG_SENSOR=y -DCONFIG_LOG=y -DCONFIG_MAIN_STACK_SIZE=4096 -DCONFIG_SHELL=y -DCONFIG_SENSOR_SHELL=y -DCONFIG_SENSOR_SHELL_STREAM=y -DCONFIG_SHELL_STACK_SIZE=4096 -DCONFIG_SENSOR_SHELL_THREAD_STACK_SIZE=4096

• Sample Output:

# One-shot readings
uart:~$ sensor get afbr_s50@0
channel type=26(distance) index=0 shift=9 num_samples=1 value=25463971078ns (1.627906)

# Streaming mode:
uart:~$ sensor stream afbr_s50@0 on data_ready incl
Enabling stream...
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=19639502441ns (1.626037)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=19720855461ns (1.625423)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=19839484581ns (1.627712)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=19920859555ns (1.628082)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20039496281ns (1.627742)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20120872615ns (1.628143)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20239500481ns (1.626922)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20320866515ns (1.625061)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20439498681ns (1.627704)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20520872341ns (1.628181)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20639492861ns (1.628036)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20720869795ns (1.626705)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20839498855ns (1.625534)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=20920871115ns (1.626144)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=21039505388ns (1.626880)
Trigger (1 / data_ready) detected
channel type=26(distance) index=0 shift=9 num_samples=1 value=21120876261ns (1.629396)
uart:~$ sensor stream afbr_s50@0 off
Disabling existing stream
[00:24:56.173,000] <wrn> AFBR_S50: OP cancelled. Stopping stream

[github-actions]

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_afbr 🆕	N/A (Added)	zephyrproject-rtos/hal_afbr@4e1eea7 (zephyr)	N/A

⛔ DNM label due to: 1 added project

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[ubieda]

Looks like github is struggling?

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

[bperseghetti]

@kartben what is process for removing an AI review block if the requested changes have been met by the PR author. I don't believe that a "click to block AI bot review" should be able to block 5 days after changes were made if there is no "click to remove a block AI bot". I believe this will ultimately aggravate reviewers and authors towards using AI bots. Also are AI bots allowed to override a maintainers approval that does not seem to follow what I thought were the project rules?

[kartben]

@kartben what is process for removing an AI review block if the requested changes have been met by the PR author. I don't believe that a "click to block AI bot review" should be able to block 5 days after changes were made if there is no "click to remove a block AI bot". I believe this will ultimately aggravate reviewers and authors towards using AI bots.

Which comment are you referring to? The Copilot reviews are not blocking ("copilot" user would need to be a Zephyr collaborator for that to be the case :) ) This PR simply needs to be approved by the assignee.

Also are AI bots allowed to override a maintainers approval that does not seem to follow what I thought were the project rules?

Can you also be more specific about this one please?

[kartben]

And apparently I also needed to revisit, so apologies that I missed the (re)-request for review!

[bperseghetti]

@kartben what is process for removing an AI review block if the requested changes have been met by the PR author. I don't believe that a "click to block AI bot review" should be able to block 5 days after changes were made if there is no "click to remove a block AI bot". I believe this will ultimately aggravate reviewers and authors towards using AI bots.

Which comment are you referring to? The Copilot reviews are not blocking ("copilot" user would need to be a Zephyr collaborator for that to be the case :) ) This PR simply needs to be approved by the assignee.

Also are AI bots allowed to override a maintainers approval that does not seem to follow what I thought were the project rules?

Can you also be more specific about this one please?

I do not see a meaningful difference between a collaborator requesting changes based on only a "manually on their side run AI review" vs integrated github copilot one. In one case the bot is the github action in the other it's more or less the collaborator who is acting as a github actions adjacent AI bot.

The only comment I see here with the block is "some gemini feedback", this over-rode the approval of the maintainer:

At a fundamental level if "AI bots are the source of truth, and can over-ride a maintainer approval" (regardless of validity of their responses or bugs caught) why even have open source projects, just make everything closed source and have AI write your code for you. When used in this way they stop being a "helpful tool in the tool chest" and become a waste of precious time and energy.

One of the fundamentals of open source in my opinion is that by the code being open humans can iterate on contributions, correct bugs and make the code base better together. If there are not strict rules placed around the timing and use of AI review bots, you will inevitably start to lose the humans and then you might as well just be closed source anyhow.

My suggestions for avoiding losing the humans and making the tools actually useful are:

1. Any AI based reviews must be done within 24 hours of a PR being opened (don't wast real reviewers time by having a bunch of possibly pedantic changes that then alter the context of an existing review)
2. Any collaborator using AI to assist at any level in their review must publicly mention in their review that the review is coming from AI.
3. Any collaborator using AI to assist at any level in their review is not able to block as they are fundamentally giving up control of their opinion to an entity that is not them and that they cannot vouch for the validity of, this by nature should invalidate their role in that particular review process.
4. AI reviews should only be suggestions in the PR process, if someone feels so convicted that there is something meaningful that the AI bot found either the author can choose at their leisure to include it or someone can create a followup PR including those changes.

[kartben]

@bperseghetti Thanks for the detailed response. In short, I would encourage you to engage on #89343 and share there some of the valuable feedback you shared here.

Longer response below.

The "some gemini feedback" comment was probably misleading, and I apologize for it. It was meant to be a disclaimer of sorts, not to imply that I was basically "only" acting as the human proxy of a bot, just blindly copy-pasting its output.

I actually did dig quite a bit into the Gemini replies, did a lot of noise filtering, pulled the datasheet and documentation, etc. before adding the comments. I thought it was fair that I would mention I was aided by a tool, but this doesn't change the fact that it's I, Benjamin, who put the -1, as a project collaborator. I would have likely spotted 10 times less issues or "smells" without Gemini, but the point is I would still have put a -1 had I considered any of them to be "serious" enough. And for better or worse, any collaborator in the project can "over-ride" (don't quite like that wording but it is what it is) maintainer approval, and this happens all the time.

One of the fundamentals of open source in my opinion is that by the code being open humans can iterate on contributions, correct bugs and make the code base better together. If there are not strict rules placed around the timing and use of AI review bots, you will inevitably start to lose the humans and then you might as well just be closed source anyhow.

Do we really need any different/additional rules than with "human" reviews though? Here, I put a -1, missed the notification about being asked to re-review and effectively left this PR stuck (or not really, since it still needed maintainer (re)approval). If @ubieda was in a hurry, he could have pinged me like we all often do, and/or the maintainer as part of their maintainer duty would have eventually noticed the stalled PR and asked me to revisit.

Any AI based reviews must be done within 24 hours of a PR being opened (don't wast real reviewers time by having a bunch of possibly pedantic changes that then alter the context of an existing review)

Again, I appreciate that my "some gemini feedback" comment was probably misleading, but this comment about "real" reviewer is actually a bit offending given that all in all I actually spent probably a whole hour reviewing that PR. The tool (or "helpful tool in the tool chest") was there to help me look in the right direction, but I invested my own time in the review.

Any collaborator using AI to assist at any level in their review must publicly mention in their review that the review is coming from AI.

Agreed, and as mentioned that's what I did here but I'll re-iterate that, in the case of this review at least, the review certainly did not come solely from AI, and dismissing it on that ground would be a mistake imo.

Any collaborator using AI to assist at any level in their review is not able to block as they are fundamentally giving up control of their opinion to an entity that is not them and that they cannot vouch for the validity of, this by nature should invalidate their role in that particular review process.

So this should really be taken to the PR I linked but by that logic we should probably also prevent collaborator to use any form of smart content assist feature in their IDE of choice for they are also giving up control?
And what should be made of static code analysis tools then? Can we "vouch" for their validity and are they 100% free of false positives? Isn't there always some cross-checking/fact-checking going on from us humans in charge?

AI reviews should only be suggestions in the PR process, if someone feels so convicted that there is something meaningful that the AI bot found either the author can choose at their leisure to include it or someone can create a followup PR including those changes.

I think that's what happened on this PR? AI comments which I wasn't quite certain about were added as review comments where I basically invited @ubieda to double check the "smell" ; others (some of which I would have spotted with a manual review too) were just put as actual request for change.

[bperseghetti]

@kartben thank you for the clarification, I will followup with any thoughts I might have around AI use in reviews on the attached issue you shared.

[ubieda]

@kartben thanks for removing your NACK. I suggest we continue the AI-related discussion on the GH issue.

@MaureenHelm would you mind revisiting this?