NXP S32 introduce support SENT #80117

congnguyenhuu · 2024-10-21T03:49:44Z

Introduce support Single Edge Nibble Transmission (SENT) driver using peripheral SENT receiver on S32Z. This driver allows to communication (read data) with SENT device.
The test result: SENT-RECEIVER-TESTING-RESULT.pptx

RFC: #83983

zephyrbot · 2024-10-21T03:50:42Z

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

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@`6b11d19`	zephyrproject-rtos/hal_nxp#455	zephyrproject-rtos/hal_nxp#455/files

⛔ DNM label due to: 1 project with PR revision

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

manuargue · 2024-11-22T04:26:59Z

@congnguyenhuu I'd suggest you to open an RFC (request for comments) and link it to this pr in order to involve community to review the proposal and reach consensus: https://docs.zephyrproject.org/latest/contribute/proposals_and_rfcs.html

JarmouniA

I see the same concerns raised in the comments #79824 (comment) and #79824 (review) in this PR.

congnguyenhuu · 2025-01-16T07:05:58Z

I see the same concerns raised in the comments #79824 (comment) and #79824 (review) in this PR.

I addressed

rruuaanng · 2025-01-17T14:06:37Z

include/zephyr/drivers/sent/sent.h

+/**
+ * @brief SENT frame structure
+ */
+struct sent_frame {


Forgive me for not being very familiar with SENT, but it doesn't look very nice, is this the final solution?
(It's nested too much.)

I improved the frame layout, merged the common fields

frame.data vs frame.slow_frame.data doesn't read nice, the first could also be understood as slow data. Right now to interpret the frame, it's necessary the information obtained from the callback via enum sent_status.

With the idea of keeping a single frame model for fast and slow frames, consider adding a frame type indicator and making fast and slow data self contained. Example:

enum sent_frame_type { SENT_FAST_FRAME, SENT_SLOW_FRAME }; struct sent_frame { enum sent_frame_type type; union { struct { uint16_t id; uint16_t data; } slow; struct { uint32_t data; } fast; }; uint32_t timestamp; uint8_t crc; };

Thanks for suggestion, I updated

decsny · 2025-01-17T15:45:39Z

This will need to go to architecture review when ready

KarstenBade · 2025-02-27T09:38:12Z

include/zephyr/drivers/sent/sent.h

The current implementation of the sent API only supports a central rx callback, whereas the low-level driver allows for the registration of separate callbacks for fast frames, serial frames, fast errors and serial errors. This design choice limits flexibility and could complicate future enhancements. I recommend revising the sent API to support multiple callbacks, aligning it more closely with the low-level driver's capabilities.

1 callback for fast frame and fast error + 1 callback for serial frame and serial error might be a good solution.

1 callback for fast frame and fast error + 1 callback for serial frame and serial error might be a good solution.

agree, this could be a good compromise.

thanks for your suggestion, I separated 2 specific callbacks for fast frame and serial frame

manuargue · 2025-02-26T06:31:59Z

samples/drivers/sent/src/main.c

+void rx_cb(const struct device *dev, uint8_t channel_id, struct sent_frame *frame,
+	   enum sent_status status, void *user_data)
+{
+	LOG_INF("Rx channel %d completed\n", channel_id);


it would be better to print sensor data and associated information rather than just "completed", e.g. timestamp, channel id, read data, status, etc.

I updated to add more information

manuargue · 2025-02-26T06:39:20Z

samples/drivers/sent/src/main.c

+
+	sent_start_rx(dev, SENT_CHANNEL);
+
+	k_sleep(K_MSEC(100));


I'd add at least a couple of iterations or an infinite loop, instead of only waiting 100ms to receive data and exiting the sample

I added an infinite loop for waiting to receive

manuargue · 2025-02-27T16:27:57Z

include/zephyr/drivers/sent/sent.h

+enum sent_status {
+	SENT_RX_SLOW_FRAME,
+	SENT_RX_FAST_FRAME,
+	SENT_RX_ERR,


With this design there's no way to distinguish between errors in slow channels and fast channels. I'd imagine it would require different level of actions whether the error occurs in one channel or the other, e.g. fast channel might be a critical impact whereas slow channel might affect only diagnostics.

I separate 2 specific error statuses for serial and fast frame

manuargue · 2025-02-27T16:35:51Z

include/zephyr/drivers/sent/sent.h

+/**
+ * @brief SENT frame structure
+ */
+struct sent_frame {


frame.data vs frame.slow_frame.data doesn't read nice, the first could also be understood as slow data. Right now to interpret the frame, it's necessary the information obtained from the callback via enum sent_status.

With the idea of keeping a single frame model for fast and slow frames, consider adding a frame type indicator and making fast and slow data self contained. Example:

enum sent_frame_type { SENT_FAST_FRAME, SENT_SLOW_FRAME }; struct sent_frame { enum sent_frame_type type; union { struct { uint16_t id; uint16_t data; } slow; struct { uint32_t data; } fast; }; uint32_t timestamp; uint8_t crc; };

include/zephyr/drivers/sent/sent.h

drivers/sent/Kconfig

congnguyenhuu · 2025-04-23T09:59:14Z

Based on the suggestions from the Architecture WG meeting on March 4th, I have updated to the v1 changes. These updates include:

Changed add_rx_callback(dev, channel, serial_cb, fast_cb, user_data) to
register_callback(dev, channel, struct saej2716_rx_callback_configs callback_configs)

With the newly defined structure for RX callback configurations in saej2716.h

struct saej2716_rx_callback_config {
	saej2716_rx_frame_callback_t callback;
	struct saej2716_frame *frame;
	uint32_t max_num_frame;
	void *user_data;
};
 
struct saej2716_rx_callback_configs {
	struct saej2716_rx_callback_config *serial;
	struct saej2716_rx_callback_config *fast;
};

RX buffers are allocated by the user to improve throughput:

Use the API register_callback to register the RX callback and allocate a user-defined RX buffer for each callback.
Count the number of RX buffer frames and trigger the RX callback when the count reaches the maximum buffer frame limit.
Reset the count when a RX error occurs or when the count reaches the maximum buffer frame limit.

Tests and samples have been updated to demonstrate usage with the new changes

…iver This driver allows to communication (receive data) with SENT device Signed-off-by: Cong Nguyen Huu <[email protected]>

enable support SAEJ2716 SENT Signed-off-by: Cong Nguyen Huu <[email protected]>

Create test, sample for saej2716 driver Signed-off-by: Cong Nguyen Huu <[email protected]>

Dat-NguyenDuy

Some comments regarding the DTS bindings and the API to be taken into account

Dat-NguyenDuy · 2025-04-28T07:59:41Z

dts/bindings/saej2716/saej2716-controller.yaml

+
+  properties:
+    reg:
+      type: array


This should be int

Dat-NguyenDuy · 2025-04-28T08:16:05Z

dts/bindings/saej2716/nxp,s32-saej2716.yaml

+
+child-binding:
+
+  compatible: "nxp,s32-saej2716-channel"


I don't think we need the compatible here

Dat-NguyenDuy · 2025-04-28T08:18:45Z

dts/bindings/saej2716/nxp,s32-saej2716.yaml

+        - 1024
+        - 2048
+      description: |
+        Specifies the number of bus timeout cycles. This value determines the maximum number


Missing explanation for what does the 0 mean.
IMO this configuration can have a default value - 0 which means timeout is disabled

Dat-NguyenDuy · 2025-04-28T08:34:05Z

include/zephyr/drivers/saej2716/saej2716.h

+/**
+ * @brief SAEJ2716 status
+ */
+enum saej2716_status {


It looks to me from saej2716_frame_type, application already knows the frame type. I don't think breaking the status for the frame type is necessary here

Dat-NguyenDuy · 2025-04-28T08:52:38Z

include/zephyr/drivers/saej2716/saej2716.h

+ * @param channel    The hardware channel of the driver instance.
+ * @param status     SAEJ2716 frame status.
+ * @param num_frame  Number of received frame.
+ * @param user_data  User data provided when the filter was added.


there is no filter in SENT as far as i know

Dat-NguyenDuy · 2025-04-28T11:00:36Z

include/zephyr/drivers/saej2716/saej2716.h

+ * @brief SAEJ2716 frame type
+ */
+enum saej2716_frame_type {
+	SAEJ2716_SERIAL_FRAME,


For serial frame, there are short format and enhanced format. Both should be supported. "SERIAL_FRAME" here is both?

Dat-NguyenDuy · 2025-04-28T11:25:38Z

dts/bindings/saej2716/saej2716-controller.yaml

+        base time unit for the system's timing operations, effectively setting the resolution
+        of the timer.
+
+    calib-method-low-latency:


There are two options for Successive Calibration Pulse Check Method: option 1 (preferred) and option 2.
Perhaps this could be successive-calib-pulse-method with two supported value 1 and 2?

Dat-NguyenDuy · 2025-04-28T11:28:58Z

dts/bindings/saej2716/saej2716-controller.yaml

+        Indicates whether the successive calibration pulse check method with low latency is enabled.
+        This method ensures faster calibration by reducing the delay between successive pulses.
+
+    calib-pulse-range-25:


This should be calib-pulse-tolerance-percent, with two supported value 20 and 25?

Dat-NguyenDuy · 2025-04-28T11:34:57Z

dts/bindings/saej2716/saej2716-controller.yaml

+        Indicates whether a 25% variation in the calibration pulse is acceptable.
+        If not, a 20% variation is acceptable.
+
+    pause-pulse-enable:


Perhaps pause-pulse-detect

Dat-NguyenDuy · 2025-04-29T03:56:45Z

dts/bindings/saej2716/saej2716-controller.yaml

+      description: |
+        Indicates whether the channel is enabled to detect a pause pulse.
+
+    crc-check-disable:


For CRC, as far as i understand:

Fast Message: CRC is optional (can be enabled/disabled). If CRC is enabled, two main options:

Legacy 4-bit CRC

Recommended 4-bit CRC

Short Serial Message: CRC seems always enabled, two options:

Legacy 4-bit CRC

Recommended 4-bit CRC

Enhanced Serial Message: 6-bit CRC

Should we do something like:

In dts bindings, create two properties for fast-crc and short-serial-crc

Define macros for these options in a header file, which can be combined to configure the CRC

zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Oct 21, 2024

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch 4 times, most recently from ec62630 to b483917 Compare October 28, 2024 03:29

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch 3 times, most recently from d00b883 to 23b01d3 Compare November 1, 2024 02:24

congnguyenhuu marked this pull request as ready for review November 22, 2024 03:42

congnguyenhuu requested a review from manuargue as a code owner November 22, 2024 03:42

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from 23b01d3 to 55d6a35 Compare November 22, 2024 03:42

zephyrbot added platform: NXP S32 NXP Semiconductors, S32 area: Samples Samples labels Nov 22, 2024

zephyrbot requested review from bperseghetti, Dat-NguyenDuy, kartben, nashif and PetervdPerk-NXP November 22, 2024 03:43

zephyrbot assigned manuargue Nov 22, 2024

congnguyenhuu marked this pull request as draft November 22, 2024 04:32

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from 55d6a35 to ec00dc8 Compare December 4, 2024 07:52

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch 5 times, most recently from c79ca4d to f2e3c0c Compare January 7, 2025 01:54

zephyrbot requested review from galak, MaureenHelm, rruuaanng and stephanosio January 15, 2025 03:16

manuargue added the RFC Request For Comments: want input from the community label Jan 15, 2025

JarmouniA reviewed Jan 15, 2025

View reviewed changes

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from 517311f to 4fc328c Compare January 16, 2025 06:57

rruuaanng reviewed Jan 17, 2025

View reviewed changes

decsny requested a review from henrikbrixandersen January 17, 2025 15:46

fabiobaltieri added DNM (manifest) This PR should not be merged (controlled by action-manifest) and removed DNM This PR should not be merged (Do Not Merge) labels Feb 4, 2025

Dat-NguyenDuy added the Architecture Review Discussion in the Architecture WG required label Feb 6, 2025

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from 4fc328c to 343b17d Compare February 6, 2025 06:09

zephyrbot requested a review from mbolivar February 6, 2025 06:10

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from 343b17d to fc21bd8 Compare February 6, 2025 09:02

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from fc21bd8 to 4e2740f Compare February 26, 2025 06:33

KarstenBade reviewed Feb 27, 2025

View reviewed changes

manuargue reviewed Feb 27, 2025

View reviewed changes

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch 2 times, most recently from f16f2bd to 85b092f Compare March 5, 2025 01:58

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from 85b092f to 725d13d Compare April 8, 2025 06:57

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from 725d13d to ac7a64c Compare April 23, 2025 09:55

congnguyenhuu added 3 commits April 25, 2025 09:54

drivers: introduce SAEJ2716 Single Edge Nibble Transmission (SENT) dr…

f832831

…iver This driver allows to communication (receive data) with SENT device Signed-off-by: Cong Nguyen Huu <[email protected]>

boards: s32z270: enable support SAEJ2716 SENT

caea133

enable support SAEJ2716 SENT Signed-off-by: Cong Nguyen Huu <[email protected]>

tests/samples: drivers: create test, sample for saej2716 driver

2a89422

Create test, sample for saej2716 driver Signed-off-by: Cong Nguyen Huu <[email protected]>

congnguyenhuu force-pushed the nxp-s32-introduce-support-sent branch from ac7a64c to 2a89422 Compare April 25, 2025 02:57

Dat-NguyenDuy reviewed Apr 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NXP S32 introduce support SENT #80117

NXP S32 introduce support SENT #80117

congnguyenhuu commented Oct 21, 2024 •

edited by Dat-NguyenDuy

Loading

zephyrbot commented Oct 21, 2024 •

edited by github-actions bot

Loading

manuargue commented Nov 22, 2024

JarmouniA left a comment

congnguyenhuu commented Jan 16, 2025

rruuaanng Jan 17, 2025

congnguyenhuu Feb 6, 2025

manuargue Feb 27, 2025 •

edited

Loading

congnguyenhuu Mar 5, 2025

decsny commented Jan 17, 2025

KarstenBade Feb 27, 2025

manuargue Feb 27, 2025

congnguyenhuu Mar 5, 2025

manuargue Feb 26, 2025 •

edited

Loading

congnguyenhuu Mar 5, 2025

manuargue Feb 26, 2025

congnguyenhuu Mar 5, 2025

manuargue Feb 27, 2025 •

edited

Loading

congnguyenhuu Mar 5, 2025

manuargue Feb 27, 2025 •

edited

Loading

congnguyenhuu commented Apr 23, 2025

Dat-NguyenDuy left a comment

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 28, 2025

Dat-NguyenDuy Apr 29, 2025

NXP S32 introduce support SENT #80117

Are you sure you want to change the base?

NXP S32 introduce support SENT #80117

Conversation

congnguyenhuu commented Oct 21, 2024 • edited by Dat-NguyenDuy Loading

zephyrbot commented Oct 21, 2024 • edited by github-actions bot Loading

manuargue commented Nov 22, 2024

JarmouniA left a comment

Choose a reason for hiding this comment

congnguyenhuu commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decsny commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

congnguyenhuu commented Apr 23, 2025

Dat-NguyenDuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

congnguyenhuu commented Oct 21, 2024 •

edited by Dat-NguyenDuy

Loading

zephyrbot commented Oct 21, 2024 •

edited by github-actions bot

Loading

manuargue Feb 27, 2025 •

edited

Loading

manuargue Feb 26, 2025 •

edited

Loading

manuargue Feb 27, 2025 •

edited

Loading

manuargue Feb 27, 2025 •

edited

Loading