drivers: sensor: Add Atmel SAM QDEC (TC) Driver #1043

jpfaff · 2017-08-08T13:23:42Z

Tested on Atmel SMART SAM E70 Xplained board
Origin: Original

mnkp · 2017-08-14T10:34:06Z

From the point of view of the existing support for Atmel SAM Family in Zephyr the driver code looks good. The only open question is if SENSOR_POSITION parameter should have a unit. In case of QDEC we measure angular rotation.

@galak, @MaureenHelm could you please review this PR.

jpfaff · 2017-08-22T13:10:49Z

@bogdan-davidoaia could you please review this PR?

zephyrproject-rtos#1043) - Call k_sched_lock() and k_sched_unlock() in callbacks to prevent other tasks from accessing the callback map/ring buffer at the same time. This becomes important for network functionality as the zephyr network stack uses multiple tasks, which can still call add/signal/remove_callback(). Signed-off-by: James Prestwood <[email protected]> * moved LOCK/UNLOCK to zjs_util.h Signed-off-by: James Prestwood <[email protected]>

jpfaff · 2017-12-05T11:34:23Z

I have updated this PR to CMake and resolved merge conflicts.

bogdan-davidoaia · 2017-12-09T10:14:13Z

@jpfaff sorry for the super late answer. Somehow the initial mails got lost in my inbox and only now did I see this PR since it got some updates.

For POSITION I'd change it to ANGULAR_ROTATION or ANGULAR_POSITION for clarity and express it in radians since we already have angular velocity expresses in radians/second for the gyro channels.

jpfaff · 2017-12-11T08:42:17Z

@bogdan-davidoaia no worries.
Different quadrature encoders have different number of steps per revolution, how would you implement that? Kconfig/DTS?
I always used rotary encorders, but it seems that there exist linear ones as well (some inkjet printers use them for example). We could stick to rotation anyway and with the steps per revolution config the linear movement can be extracted with some calculus, what do you think?

galak

I don't think drivers/sensor is the correct place for this. (not sure what is)
We need device tree support to get properties like sam_name & steps_per_unit.
Can you split out the pinmap update into its own patch from the driver.

mnkp · 2017-12-12T20:43:55Z

I don't think drivers/sensor is the correct place for this. (not sure what is)

The quadrature decoder driver indeed doesn't fit too well anywhere in the API. There was a short discussion on the devel list in August regarding the issue and the proposal was to add it to the sensor API (the other possibility would be to extend counter API).
https://lists.zephyrproject.org/pipermail/zephyr-devel/2017-August/007917.html

Can you split out the pinmap update into its own patch from the driver.

So far we've always kept pinmap in the same commit as the driver. It can be considered part of the SoC support. I just want to mention that having a separate commit for pinmap means change of style.

On a related note, should we provide pinmap also for sam4s, sam3x series?

mnkp

I have some extra comments (assuming that we want to add this driver to the sensor API and not to the counter API).

mnkp · 2017-12-12T23:12:16Z

drivers/sensor/qdec_sam/Kconfig

+	default 24
+	help
+	  Step per unit allows to set the sensor position granularity.
+	  In case of a rotary encoder this equals to steps per revolution.


Steps per unit will likely be understood by the user as steps per radian. I think we need to explain this parameter better. Something among the lines of:

"For angular position this parameter tells the number of steps per revolution (one complete turn). For linear position number of steps per unit of length. Currently only angular position is supported."

This is assuming that in the future we may want to add 'linear position' channel and want this driver to be prepared for it.

mnkp · 2017-12-15T01:33:19Z

drivers/sensor/qdec_sam/qdec_sam.c

+				degree -= 360;
+			}
+		}
+		sensor_degrees_to_rad(degree, val);


Calculating the value first in degrees and then converting it to radians means that we loose precision. We probably should make all the calculations in radians directly.

mnkp · 2017-12-15T02:00:00Z

include/sensor.h

 	/** 1.0 micro-meters Particulate Matter, in ug/m^3 */
 	SENSOR_CHAN_PM_1_0,
 	/** 2.5 micro-meters Particulate Matter, in ug/m^3 */
 	SENSOR_CHAN_PM_2_5,
 	/** 10 micro-meters Particulate Matter, in ug/m^3 */
 	SENSOR_CHAN_PM_10,
+	/** Angular position in radians */


Is this parameter supposed to change in the range 0 to 2π, -π to π? Is the range unlimited? We probably should clarify this in comment.

jpfaff · 2017-12-19T07:56:01Z

@mnkp I agree with all your comments.
What can we do to find an agreement on the best place for this kind of drivers?

In sam e70 qdec is included in the counter module.
Other socs may have specialized modules for it.
Until now drivers/sensor is used for (soc)-external components.

nashif · 2018-01-07T21:11:48Z

Until now drivers/sensor is used for (soc)-external components.

not really, lots o the sensors in drivers/sensors are part of the SoC.

If you are using the sensor.h, then that belongs into sensors.

Signed-off-by: Jonas Pfaff <[email protected]>

Tested on Atmel SMART SAM E70 Xplained board Origin: Original Jira: ZEP-2478 Signed-off-by: Jonas Pfaff <[email protected]>

codecov-io · 2018-03-06T18:37:35Z

Codecov Report

Merging #1043 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1043   +/-   ##
======================================
  Coverage    53.2%   53.2%           
======================================
  Files         436     436           
  Lines       41353   41353           
  Branches     7937    7937           
======================================
  Hits        22002   22002           
  Misses      16127   16127           
  Partials     3224    3224

Impacted Files	Coverage Δ
samples/philosophers/src/main.c	`96.87% <0%> (-1.57%)`	⬇️
kernel/posix/pthread.c	`63% <0%> (+0.5%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfc79d1...f434693. Read the comment docs.

nashif · 2018-07-03T04:12:34Z

what is the story with this PR? Is this going to be updated and is it still required? If nothing happens within the next week, it will be closed.

pdunaj · 2018-08-07T12:47:59Z

Hi, will anything happen to this PR soon? We need to use other QDEC and I don't want to generate code conflicts or implement things already done?

jpfaff · 2018-08-07T12:57:19Z

Hi, Not from my side. The project I did this for is dead and I personally have no time for it. Take whatever you can use. If needed I can delete the PR, just let me know.

…

On Tue, Aug 7, 2018, 14:48 Pawel Dunaj ***@***.***> wrote: Hi, will anything happen to this PR soon? We need to use other QDEC and I don't want to generate code conflicts or implement things already done? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1043 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC0Y1eeGj-012Iraj87cwcLiqF1qYj8fks5uOYyGgaJpZM4Owug3> .

pdunaj · 2018-08-07T13:08:04Z

Thanks, I will try to reuse the API since it was already discussed.

pdunaj · 2018-08-09T13:34:21Z

Hi @jpfaff , you can delete the PR now to avoid any conflict in the future. I will create and issue and PR with our QDEC.

jpfaff force-pushed the qdec_sam branch 3 times, most recently from 22c5d42 to fae689a Compare August 8, 2017 14:27

galak force-pushed the arm branch from fe0feea to 5398ef4 Compare August 8, 2017 16:24

jpfaff force-pushed the qdec_sam branch from fae689a to 3479214 Compare August 9, 2017 07:01

galak added this to the v1.10 milestone Aug 30, 2017

nashif modified the milestones: v1.10, v1.10.0 Oct 3, 2017

galak force-pushed the arm branch from ef1e954 to b62a353 Compare October 10, 2017 16:58

galak force-pushed the arm branch from 73de273 to 35a4c67 Compare October 20, 2017 18:55

galak force-pushed the arm branch from 8842a2b to d60d80f Compare November 3, 2017 18:54

galak self-assigned this Nov 9, 2017

galak force-pushed the arm branch 3 times, most recently from a053cce to 457699c Compare November 15, 2017 15:12

nashif modified the milestones: v1.10.0, v1.11.0 Nov 28, 2017

jpfaff force-pushed the qdec_sam branch from 3479214 to f646e54 Compare December 5, 2017 11:08

jpfaff requested review from nashif and SebastianBoe as code owners December 5, 2017 11:08

SebastianBoe removed their request for review December 5, 2017 11:47

galak force-pushed the arm branch from 3c5e735 to 7ef046f Compare December 6, 2017 14:53

jpfaff force-pushed the qdec_sam branch from f646e54 to a97d200 Compare December 7, 2017 09:08

galak force-pushed the arm branch from 28167ab to dc71422 Compare December 8, 2017 20:32

galak requested changes Dec 12, 2017

View reviewed changes

mnkp reviewed Dec 15, 2017

View reviewed changes

galak force-pushed the arm branch 2 times, most recently from ee5b12f to f95e5fd Compare January 11, 2018 22:07

jpfaff force-pushed the qdec_sam branch from 2750161 to 52533a5 Compare January 13, 2018 08:32

galak added area: ARM ARM (32-bit) Architecture platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) labels Jan 13, 2018

galak force-pushed the arm branch 2 times, most recently from 6b38c09 to ff730a4 Compare January 23, 2018 14:53

galak force-pushed the arm branch from ff730a4 to d5216a5 Compare January 29, 2018 19:42

galak removed this from the v1.11.0 milestone Feb 14, 2018

galak force-pushed the arm branch from 9e25fdc to 7170339 Compare February 23, 2018 14:53

galak force-pushed the arm branch 3 times, most recently from 23c2a89 to d3d7d54 Compare March 6, 2018 18:07

jpfaff added 2 commits March 6, 2018 12:15

drivers: sensor: Add channel POSITION

7879b6f

Signed-off-by: Jonas Pfaff <[email protected]>

drivers: sensor: Add Atmel SAM QDEC (TC) Driver

f434693

Tested on Atmel SMART SAM E70 Xplained board Origin: Original Jira: ZEP-2478 Signed-off-by: Jonas Pfaff <[email protected]>

galak force-pushed the qdec_sam branch from 52533a5 to f434693 Compare March 6, 2018 18:16

galak changed the base branch from arm to master March 6, 2018 18:16

jpfaff closed this Aug 9, 2018

jpfaff deleted the qdec_sam branch August 9, 2018 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sensor: Add Atmel SAM QDEC (TC) Driver #1043

drivers: sensor: Add Atmel SAM QDEC (TC) Driver #1043

jpfaff commented Aug 8, 2017

mnkp commented Aug 14, 2017

jpfaff commented Aug 22, 2017

jpfaff commented Dec 5, 2017

bogdan-davidoaia commented Dec 9, 2017

jpfaff commented Dec 11, 2017

galak left a comment

mnkp commented Dec 12, 2017 •

edited

Loading

mnkp left a comment

mnkp Dec 12, 2017

mnkp Dec 15, 2017

mnkp Dec 15, 2017

jpfaff commented Dec 19, 2017

nashif commented Jan 7, 2018

codecov-io commented Mar 6, 2018

nashif commented Jul 3, 2018

pdunaj commented Aug 7, 2018

jpfaff commented Aug 7, 2018 via email

pdunaj commented Aug 7, 2018

pdunaj commented Aug 9, 2018

drivers: sensor: Add Atmel SAM QDEC (TC) Driver #1043

drivers: sensor: Add Atmel SAM QDEC (TC) Driver #1043

Conversation

jpfaff commented Aug 8, 2017

mnkp commented Aug 14, 2017

jpfaff commented Aug 22, 2017

jpfaff commented Dec 5, 2017

bogdan-davidoaia commented Dec 9, 2017

jpfaff commented Dec 11, 2017

galak left a comment

Choose a reason for hiding this comment

mnkp commented Dec 12, 2017 • edited Loading

mnkp left a comment

Choose a reason for hiding this comment

mnkp Dec 12, 2017

Choose a reason for hiding this comment

mnkp Dec 15, 2017

Choose a reason for hiding this comment

mnkp Dec 15, 2017

Choose a reason for hiding this comment

jpfaff commented Dec 19, 2017

nashif commented Jan 7, 2018

codecov-io commented Mar 6, 2018

Codecov Report

nashif commented Jul 3, 2018

pdunaj commented Aug 7, 2018

jpfaff commented Aug 7, 2018 via email

pdunaj commented Aug 7, 2018

pdunaj commented Aug 9, 2018

mnkp commented Dec 12, 2017 •

edited

Loading