PWM initialization should use memory map for index #125

jadonk · 2018-08-09T05:43:13Z

Describe the bug
librobotcontrol currently relies on indexes that aren't necessarily fixed across kernel versions.

See beagleboard/linux#179 for related kernel entry.

Here's an alternative means for discovering the associated pwmchip entry for each hardware PWM.

debian@beaglebone:/var/lib/cloud9$ ls /sys/devices/platform/ocp/483*.epwmss/483*.pwm/pwm/pwmchip*
/sys/devices/platform/ocp/48300000.epwmss/48300200.pwm/pwm/pwmchip1:
device  export  npwm  power  subsystem  uevent  unexport

/sys/devices/platform/ocp/48302000.epwmss/48302200.pwm/pwm/pwmchip4:
device  export  npwm  power  subsystem  uevent  unexport

/sys/devices/platform/ocp/48304000.epwmss/48304200.pwm/pwm/pwmchip7:
device  export  npwm  power  subsystem  uevent  unexport

To Reproduce

Use the latest test build and kernel.

debian@beaglebone:/var/lib/cloud9$ rc_test_motors -s 0.3
ERROR in rc_pwm_init, can't open pwm unexport file for writing: No such file or directory
Probably kernel or BeagleBone image is too old
ERROR in rc_motor_init, failed to initialize pwm subsystem 1

Expected behavior
EduMIP motors should have turned in alternate directions.

Platform information
git:/opt/scripts/:[4a61888e8e27158bcbb0ca70531038d225a8d7af]
eeprom:[A335BNLTBLA21708EL000277]
model:[TI_AM335x_BeagleBone_Blue]
dogtag:[BeagleBoard.org Debian Image 2018-08-05]
bootloader:[microSD-(push-button)]:[/dev/mmcblk0]:[U-Boot 2018.03-00002-gac9cce7c6a]:[location: dd MBR]
bootloader:[eMMC-(default)]:[/dev/mmcblk1]:[U-Boot 2017.07-rc1-00002-g4d87f7]:[location: dd MBR]
kernel:[4.14.61-ti-r68]
nodejs:[v6.14.3]
uboot_overlay_options:[enable_uboot_overlays=1]
uboot_overlay_options:[uboot_overlay_pru=/lib/firmware/AM335X-PRU-RPROC-4-14-TI-00A0.dtbo]
uboot_overlay_options:[enable_uboot_cape_universal=1]
pkg check: to individually upgrade run: [sudo apt install --only-upgrade ]
pkg:[bb-cape-overlays]:[4.4.20180803.0-0rcnee0stretch+20180804]
pkg:[bb-wl18xx-firmware]:[1.20180517-0rcnee0stretch+20180517]
pkg:[kmod]:[23-2rcnee1stretch+20171005]
pkg:[librobotcontrol]:[1.0.0-git20180726.0-0rcnee0stretch+20180727]
pkg:[firmware-ti-connectivity]:[20170823-1rcnee1~stretch+20180328]
groups:[debian : debian adm kmem dialout cdrom floppy audio dip video plugdev users systemd-journal i2c bluetooth netdev cloud9ide gpio pwm eqep admin spi tisdk weston-launch xenomai]
cmdline:[console=ttyO0,115200n8 root=/dev/mmcblk0p1 ro rootfstype=ext4 rootwait coherent_pool=1M net.ifnames=0 quiet]
dmesg | grep pinctrl-single
[ 1.043124] pinctrl-single 44e10800.pinmux: 142 pins at pa f9e10800 size 568
dmesg | grep gpio-of-helper

jadonk · 2018-08-09T06:21:59Z

Total hack work-around just to verify the issue.

From aab43e09915f9f5c7680b4aafd9643f063418498 Mon Sep 17 00:00:00 2001
From: Demo User <[email protected]>
Date: Thu, 9 Aug 2018 06:21:05 +0000
Subject: [PATCH] Hack motor pwms

---
 library/src/io/pwm.c | 68 ++++++++++++++++++++++++++--------------------------
 library/src/motor.c  | 14 +++++------
 2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/library/src/io/pwm.c b/library/src/io/pwm.c
index 2dd2c57..808cee8 100644
--- a/library/src/io/pwm.c
+++ b/library/src/io/pwm.c
@@ -23,10 +23,10 @@
 #define unlikely(x)	__builtin_expect (!!(x), 0)
 
 // variables
-static int dutyA_fd[3];			// pointers to duty cycle file descriptor
-static int dutyB_fd[3];			// pointers to duty cycle file descriptor
-static unsigned int period_ns[3];	// one period per subsystem
-static int init_flag[3] = {0,0,0};
+static int dutyA_fd[9];			// pointers to duty cycle file descriptor
+static int dutyB_fd[9];			// pointers to duty cycle file descriptor
+static unsigned int period_ns[9];	// one period per subsystem
+static int init_flag[9] = {0,0,0,0,0,0,0,0,0};
 static int mode; // 0 for "pwmx", 1 for "pwmx:y" versions of driver
 
 /**
@@ -43,7 +43,7 @@ int __export_channels(int ss)
 	int len;
 
 	// open export file for that subsystem
-	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/export", ss*2);
+	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/export", ss);
 	export_fd = open(buf, O_WRONLY);
 	if(unlikely(export_fd<0)){
 		perror("ERROR in rc_pwm_init, can't open pwm export file for writing");
@@ -64,11 +64,11 @@ int __export_channels(int ss)
 
 	// determine mode
 	// start with channel A and also check both versions of driver
-	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/enable", ss*2); // mode 0
+	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/enable", ss); // mode 0
 	// if it exists, mode is 0
 	if(access(buf,F_OK)==0) mode=0;
 	else{
-		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/enable", ss*2, ss*2); // mode 1
+		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/enable", ss, ss); // mode 1
 		// if it exists, mode is 1
 		if(access(buf,F_OK)==0) mode=1;
 		else{
@@ -78,8 +78,8 @@ int __export_channels(int ss)
 	}
 
 	// check channel B
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/enable", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/enable", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/enable", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/enable", ss, ss); // mode 1
 	// if it exists, mode is 0
 	if(access(buf,F_OK)!=0){
 		fprintf(stderr, "ERROR in rc_pwm_init, export failed for subsystem %d channel %d\n", ss, 0);
@@ -105,7 +105,7 @@ int __unexport_channels(int ss)
 	int len;
 
 	// open export file for that subsystem
-	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/unexport", ss*2);
+	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/unexport", ss);
 	unexport_fd = open(buf, O_WRONLY);
 	if(unlikely(unexport_fd<0)){
 		perror("ERROR in rc_pwm_init, can't open pwm unexport file for writing");
@@ -139,7 +139,7 @@ int rc_pwm_init(int ss, int frequency)
 	int len;
 
 	// sanity checks
-	if(ss<0 || ss>2){
+	if(ss<0 || ss>8){
 		fprintf(stderr,"ERROR in rc_pwm_init, PWM subsystem must be 0 1 or 2\n");
 		return -1;
 	}
@@ -164,8 +164,8 @@ int rc_pwm_init(int ss, int frequency)
 	#endif
 
 	// open file descriptors for duty cycles
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/duty_cycle", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/duty_cycle", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/duty_cycle", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/duty_cycle", ss, ss); // mode 1
 	dutyA_fd[ss] = open(buf,O_WRONLY);
 
 	if(unlikely(dutyA_fd[ss]==-1)){
@@ -179,8 +179,8 @@ int rc_pwm_init(int ss, int frequency)
 		}
 	}
 
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/duty_cycle", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/duty_cycle", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/duty_cycle", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/duty_cycle", ss, ss); // mode 1
 	dutyB_fd[ss] = open(buf,O_WRONLY);
 	if(unlikely(dutyB_fd[ss]==-1)){
 		perror("ERROR in rc_pwm_init, failed to open duty_cycle channel B FD");
@@ -189,48 +189,48 @@ int rc_pwm_init(int ss, int frequency)
 	}
 
 	// now open enable, polarity, and period FDs for setup
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/enable", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/enable", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/enable", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/enable", ss, ss); // mode 1
 	enableA_fd = open(buf,O_WRONLY);
 	if(unlikely(enableA_fd==-1)){
 		perror("ERROR in rc_pwm_init, failed to open pwm A enable fd");
 		fprintf(stderr,"tried accessing: %s\n", buf);
 		return -1;
 	}
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/enable", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/enable", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/enable", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/enable", ss, ss); // mode 1
 	enableB_fd = open(buf,O_WRONLY);
 	if(unlikely(enableB_fd==-1)){
 		perror("ERROR in rc_pwm_init, failed to open pwm B enable fd");
 		fprintf(stderr,"tried accessing: %s\n", buf);
 		return -1;
 	}
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/period", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/period", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/period", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/period", ss, ss); // mode 1
 	periodA_fd = open(buf,O_WRONLY);
 	if(unlikely(periodA_fd==-1)){
 		perror("ERROR in rc_pwm_init, failed to open pwm A period fd");
 		fprintf(stderr,"tried accessing: %s\n", buf);
 		return -1;
 	}
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/period", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/period", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/period", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/period", ss, ss); // mode 1
 	periodB_fd = open(buf,O_WRONLY);
 	if(unlikely(periodB_fd==-1)){
 		perror("ERROR in rc_pwm_init, failed to open pwm B period fd");
 		fprintf(stderr,"tried accessing: %s\n", buf);
 		return -1;
 	}
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/polarity", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/polarity", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/polarity", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/polarity", ss, ss); // mode 1
 	polarityA_fd = open(buf,O_WRONLY);
 	if(unlikely(polarityA_fd==-1)){
 		perror("ERROR in rc_pwm_init, failed to open pwm A polarity fd");
 		fprintf(stderr,"tried accessing: %s\n", buf);
 		return -1;
 	}
-	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/polarity", ss*2); // mode 0
-	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/polarity", ss*2, ss*2); // mode 1
+	if(mode==0)	len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/polarity", ss); // mode 0
+	else		len = snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/polarity", ss, ss); // mode 1
 	polarityB_fd = open(buf,O_WRONLY);
 	if(unlikely(polarityB_fd==-1)){
 		perror("ERROR in rc_pwm_init, failed to open pwm B polarity fd");
@@ -300,7 +300,7 @@ int rc_pwm_cleanup(int ss)
 	char buf[MAXBUF];
 
 	// sanity check
-	if(unlikely(ss<0 || ss>2)){
+	if(unlikely(ss<0 || ss>8)){
 		fprintf(stderr,"ERROR in rc_pwm_close, subsystem must be between 0 and 2\n");
 		return -1;
 	}
@@ -309,15 +309,15 @@ int rc_pwm_cleanup(int ss)
 	}
 
 	// now open enable FDs
-	if(mode==0)	snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/enable", ss*2); // mode 0
-	else		snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/enable", ss*2, ss*2); // mode 1
+	if(mode==0)	snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm0/enable", ss); // mode 0
+	else		snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:0/enable", ss, ss); // mode 1
 	enableA_fd = open(buf,O_WRONLY);
 	if(unlikely(enableA_fd==-1)){
 		perror("ERROR in rc_pwm_cleanup, failed to open pwm A enable fd");
 		return -1;
 	}
-	if(mode==0)	snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/enable", ss*2); // mode 0
-	else		snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/enable", ss*2, ss*2); // mode 1
+	if(mode==0)	snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm1/enable", ss); // mode 0
+	else		snprintf(buf, sizeof(buf), BASE_DIR "%d/pwm-%d:1/enable", ss, ss); // mode 1
 	enableB_fd = open(buf,O_WRONLY);
 	if(unlikely(enableB_fd==-1)){
 		perror("ERROR in rc_pwm_cleanup, failed to open pwm B enable fd");
@@ -366,7 +366,7 @@ int rc_pwm_set_duty(int ss, char ch, double duty)
 	char buf[MAXBUF];
 
 	// sanity checks
-	if(unlikely(ss<0 || ss>2)){
+	if(unlikely(ss<0 || ss>8)){
 		fprintf(stderr,"ERROR in rc_pwm_set_duty, PWM subsystem must be between 0 and 2\n");
 		return -1;
 	}
@@ -409,7 +409,7 @@ int rc_pwm_set_duty_ns(int ss, char ch, unsigned int duty_ns)
 	char buf[MAXBUF];
 
 	// sanity checks
-	if(unlikely(ss<0 || ss>2)){
+	if(unlikely(ss<0 || ss>8)){
 		fprintf(stderr,"ERROR in rc_pwm_set_duty_ns, PWM subsystem must be between 0 and 2\n");
 		return -1;
 	}
diff --git a/library/src/motor.c b/library/src/motor.c
index b2b4f0b..9a1bb96 100644
--- a/library/src/motor.c
+++ b/library/src/motor.c
@@ -80,7 +80,7 @@ int rc_motor_init_freq(int pwm_frequency_hz)
 	}
 	dirB_chip[0]=MDIR1B_CHIP;
 	dirB_pin[0]=MDIR1B_PIN;
-	pwmss[0]=1;
+	pwmss[0]=4;
 	pwmch[0]='A';
 
 	// motor 2
@@ -94,7 +94,7 @@ int rc_motor_init_freq(int pwm_frequency_hz)
 		dirB_chip[1] = MDIR2B_CHIP;
 		dirB_pin[1] = MDIR2B_PIN;
 	}
-	pwmss[1]=1;
+	pwmss[1]=4;
 	pwmch[1]='B';
 
 	// motor 3
@@ -102,7 +102,7 @@ int rc_motor_init_freq(int pwm_frequency_hz)
 	dirA_pin[2]=MDIR3A_PIN;
 	dirB_chip[2]=MDIR3B_CHIP;
 	dirB_pin[2]=MDIR3B_PIN;
-	pwmss[2]=2;
+	pwmss[2]=7;
 	pwmch[2]='A';
 
 	// motor 4
@@ -110,15 +110,15 @@ int rc_motor_init_freq(int pwm_frequency_hz)
 	dirA_pin[3]=MDIR4A_PIN;
 	dirB_chip[3]=MDIR4B_CHIP;
 	dirB_pin[3]=MDIR4B_PIN;
-	pwmss[3]=2;
+	pwmss[3]=7;
 	pwmch[3]='B';
 
 	// set up pwm channels
-	if(unlikely(rc_pwm_init(1,pwm_frequency_hz))){
+	if(unlikely(rc_pwm_init(7,pwm_frequency_hz))){
 		fprintf(stderr,"ERROR in rc_motor_init, failed to initialize pwm subsystem 1\n");
 		return -1;
 	}
-	if(unlikely(rc_pwm_init(2,pwm_frequency_hz))){
+	if(unlikely(rc_pwm_init(4,pwm_frequency_hz))){
 		fprintf(stderr,"ERROR in rc_motor_init, failed to initialize pwm subsystem 2\n");
 		return -1;
 	}
@@ -325,4 +325,4 @@ int rc_motor_brake(int motor)
 		return -1;
 	}
 	return 0;
-}
\ No newline at end of file
+}
-- 
2.11.0

Solution verified via:

debian@beaglebone:/var/lib/cloud9$ rc_test_motors -m 2 -s 0.3
sending duty cycle -0.3000
sending duty cycle 0.3000
sending duty cycle -0.3000
sending duty cycle 0.3000
sending duty cycle -0.3000
sending duty cycle 0.3000
^C
calling rc_motor_cleanup()
debian@beaglebone:/var/lib/cloud9$ rc_test_motors -m 3 -s 0.3                                                                       
sending duty cycle -0.3000
sending duty cycle 0.3000
sending duty cycle -0.3000
sending duty cycle 0.3000
^C
calling rc_motor_cleanup()

StrawsonDesign · 2018-08-09T11:13:09Z

oh my, someone submitted a bug, and used the template!!!! Hallelujah, way to set a good example for the community, Jason!!!! :-D

I tried to be a little bit fancy and use globs so if the indexing changes from 0,2,4 to 0,3,7 or some other random sequence it should keep working. I just tried this with 4.9, 4.14.54 and the current 4.14.61. All seem to run with the pwmfix branch, but I'll try tomorrow with a motor to confirm before pulling into master and marking version 1.0.1 since this is a fairly important update.

My glob approach if anyone is interested: b7660f1

Also, who do we point fingers at in TI for this? I feel like this is the 4th time I've had to update the PWM interface to reflect non-backward-compatible changes in the PWM driver.

jadonk · 2018-08-09T14:52:41Z

Not really a TI thing, it is more of a Linux thing. Check the 'blame' log and maybe we can find the culprit.

https://github.com/torvalds/linux/blame/master/drivers/pwm/pwm-tiehrpwm.c
https://github.com/beagleboard/linux/blame/4.14/drivers/pwm/pwm-tiehrpwm.c

Wanted to note rc_test_drivers needed to be updated separately, since it doesn't use the library to perform the PWM driver test.

StrawsonDesign · 2018-08-10T00:51:46Z

mmm, thanks for the reminder. All sorted now and new minor version pushed out.

StrawsonDesign closed this as completed Aug 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PWM initialization should use memory map for index #125

PWM initialization should use memory map for index #125

jadonk commented Aug 9, 2018

jadonk commented Aug 9, 2018

StrawsonDesign commented Aug 9, 2018 •

edited

Loading

jadonk commented Aug 9, 2018

StrawsonDesign commented Aug 10, 2018

PWM initialization should use memory map for index #125

PWM initialization should use memory map for index #125

Comments

jadonk commented Aug 9, 2018

jadonk commented Aug 9, 2018

StrawsonDesign commented Aug 9, 2018 • edited Loading

jadonk commented Aug 9, 2018

StrawsonDesign commented Aug 10, 2018

StrawsonDesign commented Aug 9, 2018 •

edited

Loading