oPossum 1,083 Posted February 5, 2012 Share Posted February 5, 2012 The 16 channel PWM code uses too much RAM to fit in a low end MSP430. The linked list node structure is 8 bytes and 17 are required to support 16 channel PWM for a total of 136 bytes - more than the G2211/G2231 have. To reduce RAM requirements, several changes have been made. This is the old linked list node structure... typedef struct { // PWM ISR info struct unsigned time; // Time for on/off action to occur unsigned port_off; // Port pins to turn off unsigned port_on; // Port pins to turn on void *next; // Next entry in linked list } TPWM; // And this is the new one... typedef struct { // PWM ISR linked list node struct unsigned char time; // Time for on/off action to occur unsigned char port; // Port pin(s) to change void *next; // Next node in linked list } TPWM; // The number of channels has been reduced to 8 by changing the port member to 8 bit unsigned char. The separate port on/off members have been replaced by a single entry. The time has also been changed to 8 bits, so the resolution can not exceed 8 bits. Structure size is reduced to 4 bytes. Eight channels require 9 nodes, so RAM usage is only 36 bytes - a reduction of 100 bytes. Merging the port on/off members has changed the operation a bit. The ISR must determine if port pins should be turned on or off by comparing the current node to the sentinel (first) node. Port pins that are set to a PWM value of zero will be turned off and not actively driven like the 16 bit code does. A function to remove a pin from the list is no longer needed - just set the PWM value to zero. The 16 channel code used capture/compare register 1. I used that due to habit. Since the timer is in continuous mode, it is possible to use capture/compare register 0. That leaves capture/compare register 1 free for other uses such as hardware PWM or software async serial. The 16 channel ISR... __interrupt void Timer0_A1 (void) { volatile unsigned x = TAIV; // Clear interrupt flag P1OUT &= ~pa->port_off; // Port pins off P2OUT &= ~pa->port_off >> 8; // P1OUT |= pa->port_on; // Port pins on P2OUT |= pa->port_on >> 8; // pa = pa->next; // Next entry in list TACCR1 = pa->time; // Update timer compare time } The revised 8 channel ISR... __interrupt void Timer_A0 (void) { if(pa == &pw[0]) { // Sentinel node? P1OUT |= pa->port; // Port pins on } else { // P1OUT &= ~pa->port; // Port pins off } // pa = pa->next; // Next node in list TACCR0 = pa->time << 8; // Update timer compare time } Complete code with test cases #include "msp430g2211.h" #include "string.h" typedef struct { // PWM ISR linked list node struct unsigned char time; // Time for on/off action to occur unsigned char port; // Port pin(s) to change void *next; // Next node in linked list } TPWM; // TPWM pw[9], *pi; // Array and inactive list head volatile TPWM *pa; // Active node void pwm_set(const unsigned pin, const unsigned time) { const unsigned mask = 1 << pin; // const unsigned tm = time >> 8; // 8 bit time TPWM *b, *p, *n; // // // -- Try to find existing active node for this pin for(b = &pw[0], p = b->next; p != &pw[0]; b = p, p = b->next) { if(p->port & mask) { // Found it if(p->time == tm) return; // - Time is the same, nothing to do, return... while(pa != p); // Wait for this node to be active while(pa == p); // Wait for this node to become inactive // Safe to remove now if(p->port == mask) { // - Node is only used for this pin, remove it b->next = p->next; // Remove link p->next = pi; // Add to inactive list pi = p; // } else { // - Node is used for multiple pins p->port &= ~mask; // Remove this pin from the node } // break; // Found the pin, so stop searching } // } // // - Update first node in list if(tm == 0) { // If time is 0, turn off and return P1OUT &= ~mask; // return; // } else { // If time is non-zero, turn on pw[0].port |= mask; // if(time == 0xFFFF) return; // If max, no need to turn off, so return } // // // Find where the new turn off node will go for(b = &pw[0], p = b->next; p != &pw[0]; b = p, p = b->next) if(p->time >= tm) break; // Stop when an entry of >= time is found // if(p->time == tm) { // If same time, use existing node p->port |= mask; // Add this pin return; // All done... } // // n = pi; // Get a node from the inactive list pi = n->next; // // n->port = mask; // Setup new node n->time = tm; // // n->next = p; // Insert node in to active list b->next = n; // } void pwm_init(void) { unsigned n; memset(pw, 0, sizeof(pw)); // Clear entire array pa = &pw[0]; // Active list always begins with first array element pa->next = &pw[0]; // Begin with only 1 node in list pi = &pw[1]; // First inactive node is second array element for(n = 1; n < sizeof(pw)/sizeof(TPWM) - 1; ++n) // Link the inactive nodes pw[n].next = &pw[n + 1]; // // TACTL = TASSEL_2 + MC_2 + ID_0; // Setup timer, continuous mode, SMCLK/1 TACCTL0 = CCIE; // Enable timer interrupt _EINT(); // Enable interrupts } #pragma vector = TIMERA0_VECTOR __interrupt void Timer_A0 (void) { if(pa == &pw[0]) { // Sentinel node? P1OUT |= pa->port; // Port pins on } else { // P1OUT &= ~pa->port; // Port pins off } // pa = pa->next; // Next node in list TACCR0 = pa->time << 8; // Update timer compare time } void main(void) { unsigned n; unsigned o; //unsigned w[8]; //unsigned r[] = { 100, 106, 112, 118, 124, 130, 136, 142 }; WDTCTL = WDTPW | WDTHOLD; // Disable watchdog DCOCTL = 0; // BCSCTL1 = 0x80 | 15; // Run at aprox 12 MHz P1DIR = BIT0 | BIT6; // Enable outputs for LEDs on Launchpad //P1DIR = 0xFF; // Enable all P1 pins as output pwm_init(); // Initialize software PWM for(; { #if 1 pwm_set(0, n); pwm_set(6, o); n += 100; o += 123; __delay_cycles(100000); #else for(n = 0; n < 8; ++n) { pwm_set(n, w[n]); w[n] += r[n]; } __delay_cycles(100000); #endif } } EngIP, nuetron, larsie and 1 other 4 Quote Link to post Share on other sites
nobody 47 Posted February 5, 2012 Share Posted February 5, 2012 Why do You not use something like this: static, fixed table char TPWM[8][2]= //TPWM[x][0] = Port pin status; TPWM[x][1] = Time; { 0x00, 10, 0x01, 20, .... }; and routine __interrupt void Timer_A0 (void) { static char n = 0; P1OUT = TPWM[n][0]; // Port pins next state TACCR0 = TPWM[n][1] << 8; // Update timer compare time n = (n++)%8; } or updated version (skip empty lines) __interrupt void Timer_A0 (void) { static char n = 0; P1OUT = TPWM[n][0]; // Port pins next state TACCR0 = TPWM[n][1] << 8; // Update timer compare time do n = (n++)%8; while(TPWM[n][1] == 0); } EDIT: this code use 16bytes of RAM (and for 16 channel PWM need 48bytes RAM (table 16x3)) and one byte in interrupt routine. Quote Link to post Share on other sites
oPossum 1,083 Posted February 5, 2012 Author Share Posted February 5, 2012 I see several potential problems, but without having complete code, it would be unfair say anything. Please start a new thread with complete working code so your idea can be discussed without distraction. Quote Link to post Share on other sites
nobody 47 Posted February 5, 2012 Share Posted February 5, 2012 I do not need (and do not have) the full code. It is only tip for inspiration - another way to do it. If it is a bad way - no problem. I see one error in my code (need 9x2 table - first line for start impulses and max. 8 lines for ending). One sticking point - at the time of updating data in the table will be generated bad impulses. This problem can be solved by synchronization updating routine and interrupt routine. Quote Link to post Share on other sites
oPossum 1,083 Posted February 5, 2012 Author Share Posted February 5, 2012 It is only tip for inspiration - another way to do it. Using an array was considered, but has far too many problems. Using a linked list solves many of those problems. You really have to write complete working code to get a good idea of the pros and cons of a proposed solution. I have been using this method on PIC microcontrollers for many years. I developed it after looking at many other ways that where used to do mulitchannel PWM and the various problems those methods have. Quote Link to post Share on other sites
RobG 1,892 Posted February 5, 2012 Share Posted February 5, 2012 I guess it all depends on the application. For small resolution PWM, I think linked list would be an overkill. However, for >8 bit resolution, array is way too inefficient. I am doing 6bit/16ch/120Hz PWM with an array, but if I had to do 8bit/1kHz for example, I would definitely go with the linked list. Also, oPossum got me thinking and here's another solution that might work well. 3 arrays [# of channels], one to hold CCR vales, one for staging CCR vales, one to store PWM vales. During servicing of setPWM, you replace old value for that channel, order values and place them in staging array. Then you just calculate deltas and place in main array. Sorting would be the most time consuming part here. BTW, sorry for polluting you thread, will start a new thread for alternative PWMs later. Quote Link to post Share on other sites
resistor 1 Posted February 5, 2012 Share Posted February 5, 2012 I think you could save another byte per node by preallocating 9 nodes in an array and storing the next pointer as an 8 bit index into the array rather than a 16 bit pointer. In fact, you might be able to do better with bitfields since the offset really only needs 4 bits. Quote Link to post Share on other sites
oPossum 1,083 Posted February 5, 2012 Author Share Posted February 5, 2012 Yes, I have tried both. It makes the code quite a bit bigger due to the bit manipulation and/or pointer arithmetic. Using a pointer keeps the code compact and clean. Quote Link to post Share on other sites
pabigot 355 Posted February 6, 2012 Share Posted February 6, 2012 for(b = &pw[0], p = b->next; p != &pw[0]; b = p, p = b->next) { if(p->port & mask) { // Found it if(p->time == tm) return; // - Time is the same, nothing to do, return... while(pa != p); // Wait for this node to be active while(pa == p); // Wait for this node to become inactive // Safe to remove now I don't have a good handle on what constraints might be assumed in the problem and the solution, but think I see an issue. In the example, the cycle is 65536 SMCLK ticks divided into 256-tick intervals. You're willing to delay for up to a full cycle to synchronize inside pwm_set, which must be called with interrupts enabled or the synchronization spinblocks would never pass. Under these conditions it is probable, but not certain, that it's safe to remove the node. I think you're relying on the fact that the timer step is 256 SMCLK cycles to ensure you complete the manipulations before the target node becomes active again (in the worst case when there are two active nodes, and the target is only idle for one step). If there's another interrupt that executes at the "wrong" time and takes too long, that guarantee is certainly not met, and I believe you might find the timer walking the inactive list. oPossum 1 Quote Link to post Share on other sites
oPossum 1,083 Posted February 6, 2012 Author Share Posted February 6, 2012 I think you're relying on the fact that the timer step is 256 SMCLK cycles to ensure you complete the manipulations before the target node becomes active again (in the worst case when there are two active nodes, and the target is only idle for one step). If there's another interrupt that executes at the "wrong" time and takes too long, that guarantee is certainly not met, and I believe you might find the timer walking the inactive list. You are correct, this is sample code to illustrate the concept. It uses 8 bit resolution to ensure that there is enough time for that critical list manipulation code to execute. If the resolution is increased or other interrupts are active, then it would be wise to have the ISR do node removal. __interrupt void Timer_A0 (void) { if(pa == &pw[0]) { // Sentinel node? P1OUT |= pa->port; // Port pins on } else { // P1OUT &= ~pa->port; // Port pins off } // if(pa == r) { // Remove this node? // Main line code will set r to the node to be removed // Main line code will set b to the previous node pa = b->next = pa->next; // Remove this node, and make next active } else { pa = pa->next; // Next node in list } TACCR0 = pa->time << 8; // Update timer compare time } // In pwm_set()... r = p; // Mark node for removal while(b->next == p); // Wait for it to be removed Quote Link to post Share on other sites
resistor 1 Posted February 6, 2012 Share Posted February 6, 2012 Yes, I have tried both. It makes the code quite a bit bigger due to the bit manipulation and/or pointer arithmetic. Interesting. It's not surprising that the bitfield would generate big code (lots of of logical ops for doing sign/zero extensions), but I'm a little surprised that just using an array index turned out so bad. But it does! In my test, the pointer-based version took 426 bytes, while the array-indexed version took 588 bytes. One alternative that I still might experiment with is storing a signed 8-bit pointer offset in the next field. Computing the address of the next element would become "p + p->next". It'd save a byte of storage per node, and I'm hopeful that the code overhead would be less than the array-indexed version, though perhaps not as small as the baseline. Quote Link to post Share on other sites
oPossum 1,083 Posted February 6, 2012 Author Share Posted February 6, 2012 I considered using offset from beginning of array, but didn't try it. Interested in the results of your ideas. Quote Link to post Share on other sites
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.