Can SPI in Simple Libraries be speeded up?

twm47099

I want to try using the pixy SPI interface for my project with the ActivityBot in this thread:

http://forums.parallax.com/showthread.php/156970-Bluetooth-App-to-control-ActivityBot

So far I have used the pixy DAC to ActivityBot ADC which is good for tracking one object, and the UART mode to track up to 3 objects. According to the CMUCAM5 site, Pixy transmits at 1 u sec rate with a 20msec new Frame rate, and they recommend using SPI mode to capture as many Blocks of data (objects) per frame as possible. A fast transfer rate is needed to use their 'colorCode' mode that uses combinations of colors.

When I try SPI in C (from Simpletools library), the most I can capture is 2 Blocks before a new Frame starts (compared with 3 to 4 for UART).

I tried using the spin program "SPI Asm DEMO.spin" with the shiftin function just filling an array in one repeat loop and then subsequently analyzing the data array in a second repeat loop. I was able to get more than 6 blocks of data per frame. I had set the Timedelay value in SPI.Start() to 15 which gives a 1 u sec clock.

My questions:

1. Is the slow performance of SPI in the C Simple Libraries due to a clock setting (I was not able to find the source of the SPI)

2. If so, can it be changed to speed it up?

3. If so, how fast can it go, and how is it done?

(While the spin method works, I want to use C because of the other activitybot libraries, and because I don't know spin well enough to rewrite my code. -- maybe as a goal for the future, but not now.)

Thanks for your help,

Tom

kuroneko

This came up recently Faster SPI in C (GCC), how?.

twm47099

kuroneko wrote: »

This came up recently Faster SPI in C (GCC), how?.

Thanks, I remembered that there was a discussion, but I wasn't able to find it.

I was hoping that there was a way to change the clock speed to get 1MHz, but I guess not.

I've read that it is possible to translate a spin program that has PASM into C, but l'll have to do some more learning first.

Same with embedding PASM or even trying to translate the PASM Code into a C function.

But since learning something new is the reason I got involved with the Prop, it's a good thing.

Tom

jazzed

Hi.

There is no SPI library that I know of in the Simple Libraries.

This represents an opportunity for the adventurous programmer to contribute. Maybe one of the existing community libraries can be adopted?

Regardless of how it happens, I think it's good to understand what is functions are necessary to implement, and how one might go about developing a library in a reasonable way.

There is a tutorial on creating libraries, so it's hard to tell whether or not developing such an implementation is the subject of the Learn forum. If we try to keep a most helpful face and not stray from the topic, we're probably Ok.

Clearly the topic is the Parallax Simple Libraries which is written in C with Spin/PASM using SimpleIDE and Propeller C (propeller-gcc), so we should obviously stick to that. I'll help however I can with what little time I have.

twm47099

Steve,
Thanks for the info.
I tried to take the SPI tutorial and adapt it to read data from the Pixy (CMUCAM5). Getting the data from Pixy is actually easier because Pixy is always sending - no command is needed to initialize or start the transmission. However, that also causes the problem. There is no way to tell Pixy to start (or stop) sending a Frame of Blocks.

So the issue is getting the SPI read routine to read fast enough to collect a meaningful number of Blocks per Frame.

Today I looked up the shiftIn.c file and the supporting files in the SimpleTools library. I see that the code is all C (no PASM) and there doesn't appear to be any clock setting (just runs as fast as it can?) So I'm going to have to learn how to incorporate the PASM routine from the spin program I mentioned above. I know there have been threads about doing that. I'll have to find them and then I know I will have questions. I'd like to make a library to share, but since Pixy doesn't need all the features of a general SPI interface program, it may be to specialized and simple. I'm not sure if that strays too far from the Learn Forum goal.

Tom

Andy Lindsay (Parallax)

Hi Tom,

If you have not already gone through the first half of Library Studies, make sure to try that first. It's here: http://learn.parallax.com/propeller-c-library-studies.

The second half of library studies is in the queue, and I planned to address writing code that links into PASM code at the end of it using a spiffed version of the attached as an example. I'll post a more extensive write-up on it, but for now, here are a few notes:

The Test BlinkPASM.side project behaves just about the same as /libblinkpasm/Test BlinkPASM Object.spin. They both declare two instances of the PASM light blinking process, configure them differently, and modify and monitor their behaviors.

The BlinkPASM.spin library has PASM and Spin interface methods. The most important thing to note is that there are three variables shared with the PASM code: maskLed, stateLed, and halfPeriod. The cognew(@entry, @maskLed) means that the address of maskLed will appear in the ASM par register. Since they are long variables, stateLed will be in address par + 4, and halfPeriod will be in address par + 8. The ASM code reads from and writes to them.

Next, open libblinkpasm.side, click the bottom-left Show/Hide Project Manager button, and double-click blinkpasm.h, blinkpasm.c, and BlinkPASM.spin. The blinkpasm.h file has the function prototypes, but also a structure with the same variables discussed in the spin file in the previous paragraph:

typedef struct blinkpasm_struct               // Structure contains varaibles
{                                             // for cogs to share + cog & stack
  volatile int maskLed;                       // Shared
  volatile int stateLed;            
  volatile int halfPeriod;
  int cog;                                    // Remembers cog for btnblink_stop 
} blink_t;                                    // Data type for struct

Declaring variables sequentially in C does not guaranty that they will be at sequential addresses, but declaring them within a structure does. Adding typedef to the beginning and blink_t to the end allows you to declare instances of this structure and then access them either directly or by address. To make the user experience as similar to Spin as possible, blinkpasm.c creates a pointer with blink_t *device, then sets aside memory for the structure with device = (void *) malloc(sizeof(blink_t)). The blinkpasm_start function returns that address so that the application can pass it to other functions to choose which instance of the process to monitor/modify.

That's what allows Test BliknPASM.c to say blink_t *blink[2] ... blink[0] = blinkpasm_start(26, CLKFREQ/2) ... blinkpasm_getState(blink[0]) ... blinkpasm_getState(blink[1]) ... blinkpasm_setRate(blink[0], CLKFREQ/8).

Back in blinkpasm.c, note that blink_t *device receives the pointer in blinkpasm_stop, getState, and setRate. Note also the functions address the blink_t type (the structure in the .h file) with device->stateLed, device->halfPreiod, etc. Those are shared with the PASM code in the same way as maskLed, stateLed, and halfPeriod in BlinkPASM.spin.

One other key thing to make note of is the way the PASM code gets launched. This excerpt from blinkpasm.c uses BlinkPASM (the .spin filename without the extension) in two places. Be careful to insert your filename sans extension just like that, in those two spots.

    extern int binary_BlinkPASM_dat_start[];
    device->cog = 1 + cognew((void*)binary_BlinkPASM_dat_start, (void*)device);

The second argument is a pointer to the starting address of your structure, which gets passed to the PASM code's par register, just as @maskLed did in the Spin code.

Sorry, this is really terse for a large amount of info to digest. There's lots of info queued for Learn site tutorials leading up to this. Feel free to ask lots of questions.

Andy

David Betz

Andy Lindsay (Parallax) wrote: »

    extern int binary_BlinkPASM_dat_start[];
    device->cog = 1 + cognew((void*)binary_BlinkPASM_dat_start, (void*)device);

Hi Andy,

I wanted to point out that there is no reason to do the funky "1 + cog new" hack in C since it doesn't generate any better code than just testing for the return value being less than zero. Also, you don't end up having to subtract one from the cog ID when you call cogstop. I suppose there must be some reason for that technique in Spin but it always seemed surprising to me. I think you're better off just using the COG number and -1 to indicate "no COG" when writing C code.

Andy Lindsay (Parallax)

Thanks for mentioning it David.

It's not a hack to make something work in Spin, it's just a convention that was created so that the user code accessing library code would get a 0 for failure or nonzero for success. That way, user code can say:

if(blinkpasm_start(...)) {code for success} else {code for failure}.

Andy

twm47099

Andy,
Thanks for the info. Doing a quick read of your post, I'd say it's well over my head. But it is something I've been wanting as I learn Prop C. It will take some careful reading and exploring. I've been wondering where structures would be useful (otherwise to a nubie it seems like a lot of typing -- maybe another future tutorial)

Is this method of including PASM the same as was used for the C version of full duplex serial? (It is useful to have a moderately complex working example.)

Tom

Andy Lindsay (Parallax)

Hi Tom,

Yeah, Library Studies Part 2 will depend on a lot more background material, like passing pointers to/from functions, structures, typedefs and casting.

The FDS library actually has an extra level on top of what we have in blinkpasm because FDS is designed to play nice with the simpletext library. So, its structure also has some function pointers for serial transmit and receive that dprint, dscan, and friends can use. But aside from that, I think I actually started with FDS to make my first draft of an ASM example. It also had a button, and in this second example, I removed the button because the light blinking can still have two-way communication between cog and app.

As a starting point, examine Test BlinkPASM Object.spin and BlinkPASM.spin. Try following from the Test BlinkPASM Object.spin calls to BlinkPASM.spin Spin functions. Watch how those functions set and/or check global variables. Then, look at how cogstart sends the address of the first global variable to the ASM code's par variable. Keep the Propeller Manual on hand, and look up things things as you go. If you get stuck or would like clarification on any point, just ask in this thread. BTW, the posts about C + PASM and Spin + PASM will get migrated to a second thread soon.

Andy

David Betz

Andy Lindsay (Parallax) wrote: »

Thanks for mentioning it David.

It's not a hack to make something work in Spin, it's just a convention that was created so that the user code accessing library code would get a 0 for failure or nonzero for success. That way, user code can say:

if(blinkpasm_start(...)) {code for success} else {code for failure}.

Andy

Okay, someone told me once that it had to do with the fact that testing for zero was slightly more efficient than testing for less than 0. Anyway, I've been confused in the past by variables called "cogID" that were not really the COG ID but rather the COG ID + 1. Your example would still work if you just changed it to:

if(blinkpasm_start(...) >= 0) {code for success} else {code for failure}.

and you'd have the advantage that you'd really be returning a COG ID.

Andy Lindsay (Parallax)

Hmmm, now that you mention it, I remember hearing a something like that too. It doesn't make sense to me though, because adding and subtracting 1 seems like it would negate any increases in efficiency. I'll check with the guys who set it up. At any rate, you are saying that there isn't any code overhead difference between if(variable) and if(variable <= value) in Propeller GCC, is that right? If that's the case, the question for the guys would be if they want to stick with the convention from the API standpoint.

David Betz

Andy Lindsay (Parallax) wrote: »

Hmmm, now that you mention it, I remember hearing a something like that too. It doesn't make sense to me though, because adding and subtracting 1 seems like it would negate any increases in efficiency. I'll check with the guys who set it up. At any rate, you are saying that there isn't any code overhead difference between if(variable) and if(variable <= value) in Propeller GCC, is that right? If that's the case, the question for the guys would be if they want to stick with the convention from the API standpoint.

I doubt there is any code overhead difference between ==0 and <0 tests. I guess we'd have to try specific cases to find out for sure,

twm47099

Andy,
Just an update --
I've run the Spin code (once I fixed my indentation mistake), and follow what both the PASM and spin code are doing. It's given me encouragement that I'll be able to follow the spin and PASM for the SPI objects.

The C code for the blinkPASM files seems pretty complex, and I will have to go through Part 1 of the library Tutorials slowly and carefully. I am looking forward to the Part 2 tutorial with all your explanations.

Tom

jazzed

Hi Tom,

I recommend the 15 minute rule. If you can't answer a question after researching it for 15 minutes, ask it here. Then someone can show you how to find the answer. This can also help others find the answer. You will get an answer one way or another with less headaches (hopefully).

Andy mentions structures in C. Structures (also called records in other languages) are necessary for ordering data in memory. They also provide for convenient access using '.' dot or '->' pointer notation (don't let pointer give you a headache, it is actually quite easy). Andy should consider providing a pointer tutorial at some point.

One reason structures are necessary in C instead of using a set of variables is that the compiler is free to optimize code for us. If you have 4 variables in a row and rely on the position of any of them, then 1) the addresses can be completely backwards, and 2) if you don't use one of the variables, it will change the addresses. The alternative is to use arrays with indices to organize information ... which usually turns into horrible code. Also, using a struct makes working with a set of variables easier and less error prone.

Andy Lindsay (Parallax)

Tom,

Good deal. After Library Studies part 1, it'll seem more approachable, but there will still be some question marks. I'm working on filling those in, and will try to keep up with you.

Andy

Andy Lindsay (Parallax)

jazzed wrote: »

...Andy mentions structures in C. Structures (also called records in other languages) are necessary for ordering data in memory. They also provide for convenient access using '.' dot or '->' pointer notation (don't let pointer give you a headache, it is actually quite easy). Andy should consider providing a pointer tutorial at some point...

Yep, it's at the top of my queue.

twm47099

jazzed wrote: »

Hi Tom,

I recommend the 15 minute rule. If you can't answer a question after researching it for 15 minutes, ask it here. Then someone can show you how to find the answer. This can also help others find the answer. You will get an answer one way or another with less headaches (hopefully).

...
One reason structures are necessary in C instead of using a set of variables is that the compiler is free to optimize code for us. If you have 4 variables in a row and rely on the position of any of them, then 1) the addresses can be completely backwards, and 2) if you don't use one of the variables, it will change the addresses. The alternative is to use arrays with indices to organize information ... which usually turns into horrible code. Also, using a struct makes working with a set of variables easier and less error prone.

Thanks for the 15 minute rule, I'll certainly be asking questions.

Re structures, I've been using the alternate array method (not for ordering variables in memory, but for grouping variables by subject). And it does get unruly and requires a lot of commenting so that I can figure out what I did a week later.

Tom

twm47099

A little more progress today. I took the BlinkPASM files that Andy attached to post 6, deleted the CMM folder, retyped the c and h files, and rebuilt the library using the methods from the Library Studies tutorial. It worked. I was surprised that I didn't seem to have to do anything with the SPIN file other than adding it to the project in the project manager window and building with Create Project Library checked in the Linker tab. There are a lot of steps, but following a list of steps it was pretty straightforward.

Am I missing something?

If not, then next step is to write the c programs/headers for SPI_Asm.spin and see what happens.

Tom

Andy Lindsay (Parallax)

Hi Tom,

Congratulations, you have made good progress. Yes, since the .spin file was added to the project's file list, it is part of the project. The .h and .c files exchange data with the PASM code the same way the spin methods in the object do. Keep in mind that if a variable needs to be shared with the ASM Code, it should be added to the struct in the .h file. Try looking up the code for each note in post #6. Some of it will start to make sense, but other parts will probably lead to more questions. Remember the 15 minute rule.

Andy

twm47099

Andy Lindsay (Parallax) wrote: »

Hi Tom,

Congratulations, you have made good progress. Yes, since the .spin file was added to the project's file list, it is part of the project. The .h and .c files exchange data with the PASM code the same way the spin methods in the object do. Keep in mind that if a variable needs to be shared with the ASM Code, it should be added to the struct in the .h file. Try looking up the code for each note in post #6. Some of it will start to make sense, but other parts will probably lead to more questions. Remember the 15 minute rule.

Andy

Andy,
My next step is to understand the SPI_Pasm.spin and demo programs. They don't appear to use the methods in the example of post 6 for synchronizing the variables between the Spin and PASM parts of the program.


I expect that I will have to read more of the Prop manual, and write out the steps of the PASM part (what each step is doing) for the intro part and the update_shiftin section.

I've thought about just adding a section to the spin code that declares the arg variables in relation to par, but I think I should try to understand what is going on first.

Question-- I don't see how 'value' in the spin SHIFTIN function gets associated with arg4 in the PASM code. Could you explain or point me to some info?

Thanks
Tom

Andy Lindsay (Parallax)

The start function gives the address of a variable named command to the ASM code with this:

okay := cog := cognew(@loop, @command) + 1

If the app calls shiftout, shiftout in turn passes the address of Dpin to setcommand. The main ram byte address of Cpin is Dpin + 4. The address of Mode is Dpin + 8, etc through value, which is Dpin + 16

PUB SHIFTOUT(Dpin, Cpin, Mode, Bits, Value) 
    setcommand(_SHIFTOUT, @Dpin)

setcommand makes the command variable so that the address is carried in the lower 16 bits, and the command info is in the upper 16 bits. Remember, the address of command is what with to the ASM code in the par register, so the ASM watches to find when it gets populated with information.

PRI setcommand(cmd, argptr)
    command := cmd << 16 + argptr 
    repeat while command

Andy Lindsay (Parallax)

I don't know if we can trust parameters to have sequential addresses in C. You could test with print("&param1 = %d, &param2 = %d\n", param1, param2). If yes, the port can be more mechanical. If no, then your functions will need to copy the parameters to the struct, like the functions do in post #6. I'm out the rest of the day, more on this tomorrow.

twm47099

Andy,
Thanks for the info.
Last night I tried to understand the details of the beginning of the PASM code (up to the jumps label). I was not successful and came away with a lot of questions (such as why 'add :arg, d0' -- adding $200 to a label?, why such a complicated routine leading up to the 'jump t2', and more). But I think those need to be asked in the Prop 1 forum and probably after I learn some more about pasm.

Based on your last answer, I think the variables that need to be in the structure are:
Command
Clockdelay
Clockstate
Dpin
Cpin
Mode
Bits
Value
Flag

I'm not sure about what to do with the constants(MSBPRE, etc.). I think they can just be declared and intialized, but don't need to be in the structure. Is that correct?

I also think my next step is to port the spipasm.spin code to C, add a simplified version of the demo program (I don't have the temperature sensor that is used in the spin demo, so I will use Pixy for the demo / test harness code), and add the function prototypes into one file (the spin file with the pasm will be added to the project. Once I get that running, I will break the program into separate sections (header, test harness, spipasm.c) and build a library.

I expect to have questions as I go along.
If anything I've written doesn't make sense, please correct me.

thanks again
Tom

Andy Lindsay (Parallax)

twm47099 wrote: »

...But I think those need to be asked in the Prop 1 forum and probably after I learn some more about pasm.

That sounds like a good plan. In the meantime, the PASM can be treated like a black box so long as we are clear on the variables and addressing in the Spin code.

Based on your last answer, I think the variables that need to be in the structure are:
Command
Clockdelay
Clockstate
Dpin
Cpin
Mode
Bits
Value
Flag

Looks good. I'd recommend adding cog. Probably like this:

typedef struct spi_struct                     // Structure contains varaibles
{                                             // for cogs to share + cog & stack
  volatile int command;                       // Shared
  volatile int clockDelay;            
  volatile int restState;
  int dPin;
  int cPin;
  volatile int mode;
  volatile int bits;
  volatile int value;
  volatile int flag;
  int cog;                                    // Remembers cog for spi_stop 
} spi;                                        // Data type for struct

I'm not sure about what to do with the constants(MSBPRE, etc.). I think they can just be declared and intialized, but don't need to be in the structure. Is that correct?

Excellent question. It's a little tricky because the enumeration for the same constants is different in the simpletools library. Let's start with this and see how it goes...

#ifndef   MSBPRE     
/**
 * @brief For use with shift_in.  Stands for most significant bit first, pre-clock.
 */
#define   MSBPRE     0
#endif

#ifndef   LSBPRE     
/**
 * @brief For use with shift_in.  Stands for least significant bit first, pre-clock.
 */
#define   LSBPRE     1
#endif

#ifndef   MSBPOST    
/**
 * @brief For use with shift_in.  Stands for most significant bit first, post-clock.
 */
#define   MSBPOST    2
#endif

#ifndef   LSBPOST    
/**
 * @brief For use with shift_in.  Stands for least significant bit first, post-clock.
 */
#define   LSBPOST    3
#endif
  
// Values for use with shift_out
#ifdef   LSBFIRST  
#undef    LSBFIRST 
#endif
/**
 * @brief For use with shift_out.  Stands for least significant bit first.
 */
#ifndef LSBFIRST
#define   LSBFIRST   4
#endif

#ifdef   MSBFIRST   
#undef    MSBFIRST 
#endif
/**
 * @brief For use with shift_out.  Stands for most significant bit first.
 */
#ifndef   MSBFIRST   
#define   MSBFIRST   5
#endif

/// @cond DoNotDoc

#ifndef   _SHIFTOUT    
#define   _SHIFTOUT  1
#endif

#ifndef   _SHIFTIN    
#define   _SHIFTIN  2
#endif

/// @endcond

I also think my next step is to port the spipasm.spin code to C, add a simplified version of the demo program (I don't have the temperature sensor that is used in the spin demo, so I will use Pixy for the demo / test harness code), and add the function prototypes into one file (the spin file with the pasm will be added to the project. Once I get that running, I will break the program into separate sections (header, test harness, spipasm.c) and build a library.

Agreed. I'd recommend using the blinkpasm project as a template. Do a Save Project As -> libspipasm, saving it into a folder named libspipasm. You can use a file program (Windows Explorer, Mac Finder) to rename blinkpasm.c to spipasm.c, and blinkpasm.h to spipasm.h. Make sure to copy SPI_Asm.spin into the folder too. Next, use SimpleIDE's bottom-left Show/Hide Project button to view the project's files. Click and delete blinkpasm.h, blinkpasm.c, and BlinkPASM.spin. Then, use Project->Open Tab to Project to add spipasm.h, spipasm.c, and SPI_Asm.spin.spin.

In spipasm.h, you'll to update #ifndef __BLINKPASM_H to #ifndef __SPIPASM_H and #define __BLINKPASM_H to #define __SPIPASM_H. After #ifdef __cplusplus, extern "C", {, #endif, add the following:

- The #define MSBPRE, LSBPRE, etc constants from the snippet above.
- The struct from above.
- Your function prototypes. Here is one way that might turn out to be useful:

spi *spi_start(int dPin, int cPin, int delay, int restState); 

void spi_stop(spi *device);

int spi_in(spi *bus, int mode, int bits);

void spi_out(spi *bus, int mode, int bits, int value);

Note that I did not add setcommand. I'd recommend adding its function prototype at the top of spinpasm.c. It'll probably have to take the form: void setcommand(spi *bus, int cmd, int *argptr);

A hint on the porting for spipasm.c.

spi *spi_start(int dPin, int cPin, int clockDelay, int restState) 
{
  spi *bus;                                            // Declare buttonblink pointer
  bus = (void *) malloc(sizeof(bus));              // Allocate memory for it

  bus->clockDelay = clockDelay;            
  bus->restState = restState;

  // ...more here, don't forget the extern int and cognew.

twm47099

I had started writing the combined program and had added 'int cog' to the structure. But based on your post above I have a couple of simple questions.

1. In the blink program you used blink_t as the data type for the structure. I've seen other code that uses the _t suffix for a structure data type. I thought that it was necessary. Is that just an editorial convention? I had been useing spia_t in my code. But I like less typing.

2. I'm not sure if any of the function names have to be the same as those in the spin file. The function names in blinkpasm.c were the same. I did save SPI_ASM.spin into another file SPIAsm.spin since I wasn't sure if the underscore in the name would cause a problem.

3. I'm not sure if I called some of the parameters correctly in the C program.
-- In the functions spipasm_SHIFTOUT and spipasm_SHIFTIN I'm not sure how to write the address of Dpin in the calls to spipasm_setcommand.
-- In spipasm_SHIFTIN I'm not sure if I wrote the return of Value correctly.
-- in spipasm_SHIFTIN and in spipasm_setcommand I'm not sure if I wrote the 'while' correctly. The purpose of that command is to delay until the PASM clears 'Flag' (for the SHIFTIN) or 'command' for setcommand.

I didn't define the constants in this try, but just use the value in the function calls.

The code (very draft) is shown below. I've tried compiling it and it built ok (once I fixed the capitalization in the call to spipasm_SHIFTIN.) I haven't actually tried running it yet.

I did get some warnings. I copied them below the code.

I'm not sure about my use of parameters noted above.

Tom

/* spipasm - 
 */
 #include "simpletools.h"                      // Library includes
  
 typedef struct spipasm_struct               // Structure contains varaibles
 {                                             // for cogs to share + cog & stack
   volatile int command;                       // Shared
   volatile int ClockDelay;            
   volatile int ClockState;
   volatile int Dpin;
   volatile int Cpin;
   volatile int Mode;
   volatile int Bits;
   volatile int Value;
   volatile int Flag;
   int cog;                                    // Remembers cog for spiasm_stop 
 } spia_t;                                    // Data type for struct
  
 int pixydata[200];
  
 // function prototypes
  
 spia_t *spipasm_start(int CLKDLY, 
                          int CLKST);

 void spipasm_stop(spia_t *device);
  
 void spipasm_SHIFTOUT(spia_t *device,
                     int DQ, int CLK, int MD, int BIT, int VLU);
  
 int spipasm_SHIFTIN(spia_t *device,int DQ, int CLK, int MD, int BIT);
  
 void spipasm_setcommand(spia_t *device, int cmd, int *argptr);
  
 /*
   libblinkpasm.c
   Test harness for the spipasm library. 
 */
  
 // #include "spipasm.h"
  
 spia_t *spia; 
  
 int main()                                    // Main function
 {   
 // Put demo code here
                   //  spia = spipasm_start(ClockDelay, ClockState)
   spia = spipasm_start(15, 0);    // Launch, get handles
   int i = 1;
   while(i<198)    //  fill pixydata array with spi bytes
   {
                   //  spipasm_shiftin(Dpin, Cpin, Mode, Bits)
     pixydata[i] = spipasm_SHIFTIN(spia, 0, 2, 0, 8);  // Mode 0 = MSBPRE
     i++;
   }
   i = 1;
   while(i<198)    //  write pixydata array
   {
   printi("color = %d\n", pixydata[i]);
   i++;
   }
 spipasm_stop(spia);
 }
  
 // These are the actual functions
  
 //  ****************************************
 spia_t *spipasm_start(int CLKDLY, 
                          int CLKST)
 {
   spia_t *device;                                    // Declare spipasm pointer
   device = (void *) malloc(sizeof(spia_t));         // Allocate memory for it  
 

   device->ClockDelay = CLKDLY;
   device->ClockState = CLKST;
   if(device->cog == 0)
   {
     device->command = 0;
     extern int binary_SPIAsm_dat_start[];
     device->cog = 1 + cognew((void*)binary_SPIAsm_dat_start, (void*)device);
   }
   return device;                                       // Return pointer to structure
 } 

 //  ************************************
 void spipasm_stop(spia_t *device)
 {
   if(device->cog > 0)                                  // If cog running
   {
     cogstop(device->cog - 1);                          // Shut down cog
     device->cog = 0;                                   // Clear cog varaible
     free(device);                                      // Release allocated memory
   }  
 } 

 //  ************************************
 void spipasm_SHIFTOUT(spia_t *device, int DQ, int CLK, int MD, int BIT, int VLU)
 {
   device->Dpin = DQ;
   device->Cpin = CLK;
   device->Mode = MD;
   device->Bits = BIT;
   device->Value = VLU;
   spipasm_setcommand(spia,1, device->Dpin);
 //  device->command = 1 << 16 + device->Dpin;
 //  while(device->command);
 } 

 //  ************************************
 int spipasm_SHIFTIN(spia_t *device,
                     int DQ, int CLK, int MD, int BIT)
 {
   device->Dpin = DQ;
   device->Cpin = CLK;
   device->Mode = MD;
   device->Bits = BIT;
   spipasm_setcommand(spia, 2, device->Dpin); 
 //  device->command = 1 << 16 + device->Dpin;
 //  while(device->command);
 //
   while(device->Flag);
   return device->Value;
 }
  
 //  ************************************
   void spipasm_setcommand(spia_t *device, int cmd, int *argptr)
 {
   device->command = cmd << 16 + *argptr;
   while(device->command);
 }  
  

Build messages:

 

 propeller-elf-gcc.exe -v GCC 4.6.1 (propellergcc_v1_0_0_2162)
 openspin.exe -c -I M:/Program Files/SimpleIDE/bin/../propeller-gcc/spin/ -o cmm/SPIAsm.dat SPIAsm.spin
 Propeller Spin/PASM Compiler 'OpenSpin' (c)2012-2013 Parallax Inc. DBA Parallax Semiconductor.
 Compiled on Nov  7 2013
 Compiling...
 SPIAsm.spin
 Done.
 Program size is 636 bytes
 propeller-elf-objcopy -I binary -B propeller -O propeller-elf-gcc --redefine-sym _binary_cmm_SPIAsm_dat_start=_binary_SPIAsm_dat_start --redefine-sym _binary_cmm_SPIAsm_dat_end=_binary_SPIAsm_dat_end --redefine-sym _binary_cmm_SPIAsm_dat_size=_binary_SPIAsm_dat_size cmm/SPIAsm.dat cmm/SPIAsm_firmware.o
 propeller-elf-ar.exe rs cmm/spipasm (1).a cmm/SPIAsm_firmware.o
 propeller-elf-ar.exe: creating cmm/spipasm (1).a
 propeller-elf-gcc.exe -I . -L . -I M:/Mdrive Documents/robot/SimpleIDE/Learn/Simple Libraries/Utility/libsimpletools -L M:/Mdrive Documents/robot/SimpleIDE/Learn/Simple Libraries/Utility/libsimpletools/cmm/ -I M:/Mdrive Documents/robot/SimpleIDE/Learn/Simple Libraries/TextDevices/libsimpletext -L M:/Mdrive Documents/robot/SimpleIDE/Learn/Simple Libraries/TextDevices/libsimpletext/cmm/ -I M:/Mdrive Documents/robot/SimpleIDE/Learn/Simple Libraries/Protocol/libsimplei2c -L M:/Mdrive Documents/robot/SimpleIDE/Learn/Simple Libraries/Protocol/libsimplei2c/cmm/ -o cmm/spipasm (1).elf -Os -mcmm -m32bit-doubles -fno-exceptions -std=c99 spipasm (1).c cmm/spipasm (1).a -lm -lsimpletools -lsimpletext -lsimplei2c -lm -lsimpletools -lsimpletext -lm -lsimpletools -lm
 spipasm (1).c: In function 'spipasm_SHIFTOUT':
 spipasm (1).c:106:3: warning: passing argument 3 of 'spipasm_setcommand' makes pointer from integer without a cast [enabled by default]
 spipasm (1).c:35:6: note: expected 'int *' but argument is of type 'int'
 spipasm (1).c: In function 'spipasm_SHIFTIN':
 spipasm (1).c:120:3: warning: passing argument 3 of 'spipasm_setcommand' makes pointer from integer without a cast [enabled by default]
 spipasm (1).c:35:6: note: expected 'int *' but argument is of type 'int'
 propeller-load -s cmm/spipasm (1).elf
 propeller-elf-objdump -h cmm/spipasm (1).elf
 Done. Build Succeeded!
 

Andy Lindsay (Parallax)

Ah, so you are putting all the ingredients in a single file first. It looks like you're getting close to a point where you'll be able to test in run-time. Just a few compiler errors to fix (discussed in 3).

1. The _t suffix is a shorthand indicating that it's a typedef and is considered a good coding practice. Likewise with the _st for structure names. When I want to make a more beginner-friendly API, I leave out those suffix because the user code becomes:

spi *bus = spi_start(etc...);

...which is a step down from:

spi_t *bus = spi_start(etc...);

2. The function names can be whatever you make them. The only thing the functions have to do is make sure a set of variables in the structure have the correct info before copying the address of the structure's Dpin element to the structure's command element. The examples I posted were (out of habit) experimenting with a user-friendly API style.

3.

The way you set up the return value in spipasm_SHIFTIN looks correct.

Notice that the compile errors are all complaining about argument 3 of the spinpasm_setcommand function, saying that they are receiving an int value instead of an int address. I think you can fix that by passing the address &device->Dpin instead of just the value at the device->Dpin address. In other words, just precede device->Dpin with an ampersand like this: &device->Dpin.

In the code below, the third argument is correctly set up to receive an address with int *argptr. But, cmd << 16 + *argptr is adding the value at the argptr address, not the address itself. I think it would be better to change it to device->command = (int) argptr | (cmd << 16). I would also change the add (+) to bit-wise or ( | ) in the expression because the intent of the operation is logical, not arithmetic. The spin code relies on the fact that both work if the target variable starts as zeros, but there are situations where you might try to use add (+) on a variable with bits that are already set, which will result in potential carry operations and unintentionally setting higher order bits in the variable. The bitwise or ( | ) operation will not carry to higher order bits.

   void spipasm_setcommand(spia_t *device, int cmd, int *argptr)     // OK
 {
   // device->command = cmd << 16 + *argptr;             // See notes above
   device->command = (int) argptr | (cmd << 16)
   while(device->command);
 }

jazzed

Hi.

Just some observations ....

At this point the library is still primitive. It is also more of an application than a library. At some point main needs to be split out of the library so that other applications having their own main can use the library. So the library, assuming a name like libspia, shoul have files like this:

1. libspia.c : contains the "unit test" code main() for the library and only uses the spia_ functions.
2. spia.c : contains the spia_ functions and global data if any.
3. spia.h : contains the structure and other type definitions, forward function declarations, and documentation for the functions.
4. SPIAsm.spin : contains the spin/pasm code.
5. libspia.side : the project library which the IDE maintains.

About performance ....

Just reading and writing one value at a time will never allow for the best possible performance because of function call overhead. One really needs to consider using buffers for reading and writing more than one byte at a time. I recommend something like this.

/**
 * @brief Reads max length bytes from the spi device into the buffer.
 *
 * The spia_t device should have the IO pin fields previously initialized by the start function.
 * Also any options such as 3/4 wire operation, etc... should be set in the spia_t device struct.
 *
 * @param dev This is the main control structure that contains SPIA device details passed from the start (or open) function.
 * @param maxlen This is the maximum number of bytes allowed to be read into the buffer to prevent buffer overflows.
 * @param [out] buffer This unsigned char buffer pointer contains the start address of the buffer where data read will be placed. The contents of the buffer is an output.
 * @returns The number of bytes actually read into the buffer if > 0. If 0, no bytes were read. If < 0, a read error occurred.
 */
int spia_read(spia_t *dev, unsigned char *buffer, int maxlen);

/**
 * @brief Write length bytes from the buffer into spi device.
 *
 * The spia_t device should have the IO pin fields previously initialized by the start function.
 * Also any options such as 3/4 wire operation, etc... should be set in the spia_t device struct.
 *
 * @param dev This is the main control structure that contains SPIA device details passed from the start (or open) function.
 * @param length This is the number of bytes to be written from the buffer into the SPIA device.
 * @param buffer This unsigned char buffer pointer contains the start address of the buffer where data read will be placed. The contents of the buffer is an input that must be set before calling the function.
 * @returns The number of bytes actually written into the buffer if > 0. If < 1, an error occurred; 0 means no bytes were written.
 */
int spia_write(spia_t *dev, unsigned char *buffer, int length);

Please try to get into a habit of specifying what a function should do as above before writing the function. This accomplishes several important things.

1. Creates a plan for what your code should do when it's done.
2. Creates a contract between your library functions and the user.
3. Documents the function immediately so you don't forget to do it later (must be checked for accuracy later).
4. Provides a forward declaration of the function for the compiler to check for errors.
5. Provides for "single source" of documentation in the code and for the help pages to reduce maintenance burden for everyone.
6. Helps you and others understand the benefits of doing this over time.

twm47099

Andy,
Thanks for your help.

Ah, so you are putting all the ingredients in a single file first. It looks like you're getting close to a point where you'll be able to test in run-time. Just a few compiler errors to fix (discussed in 3).

I thought that keeping every thing in the same file until I got it running would be best, then I'll break it into the library components.

1. The _t suffix is a shorthand indicating that it's a typedef and is considered a good coding practice. Likewise with the _st for structure names. When I want to make a more beginner-friendly API, I leave out those suffix ...

I like the beginner's way better. Once it is working I may change it.

The function names can be whatever you make them. The only thing the functions have to do is make sure a set of variables in the structure have the correct info before copying the address of the structure's Dpin element to the structure's command element. The examples I posted were (out of habit) experimenting with a user-friendly API style.

I wasn't sure. I still get mixed up on how this is working. I recommend stressing that in the future tutorial, especially if examples use the same function names as the spin object.

So the only thing that needs to be the same spelling is the spin file name in the 2 statements in the open function?

       extern int binary_SPIAsm_dat_start[];
     device->cog = 1 + cognew((void*)binary_SPIAsm_dat_start, (void*)device);

Can the spin file name have an underscore in it like the original API_Asm.spin did?

I made the changes to the calling functions (put & before device->) and to spipasm_setcommand (changed device->command = cmd << 16 + *argptr;
to device->command = (int) argptr | (cmd << 16);

but I was still getting warnings complaining about:

spipasm.c:35:6: note: expected 'int *' but argument is of type 'volatile int *'

I looked back at your examples and saw that you did not declare the dPin or cPin to be volatiles. So I deleted volatile from my definitions of the pins in the structure. It compiled w/o any errors or warnings. It will be later today before I have the opportunity to test with the Pixy.

The "error free??" code is posted below:

/*
 spipasm - 
 */
 #include "simpletools.h"                      // Library includes 

 typedef struct spipasm_struct               // Structure contains varaibles
 {                                             // for cogs to share + cog & stack
   volatile int command;                       // Shared
   volatile int ClockDelay;            
   volatile int ClockState;
   int Dpin;
   int Cpin;
   volatile int Mode;
   volatile int Bits;
   volatile int Value;
   volatile int Flag;
   int cog;                                    // Remembers cog for spiasm_stop 
 } spia_t;                                    // Data type for struct
  
 int pixydata[200];
  
 // function prototypes
  
 spia_t *spipasm_start(int CLKDLY, 
                          int CLKST); 

 void spipasm_stop(spia_t *device);
  
 void spipasm_SHIFTOUT(spia_t *device,
                     int DQ, int CLK, int MD, int BIT, int VLU);

 int spipasm_SHIFTIN(spia_t *device,int DQ, int CLK, int MD, int BIT);
  
 void spipasm_setcommand(spia_t *device, int cmd, int *argptr);
  
 /*
   libblinkpasm.c
   Test harness for the spipasm library. 
 */ 

 // #include "spipasm.h"
  
 spia_t *spia; 
  
int main()                                    // Main function
 {   
 // Put demo code here
                   //  spia = spipasm_start(ClockDelay, ClockState)
   spia = spipasm_start(15, 0);    // Launch, get handles
   int i = 1;
   while(i<198)    //  fill pixydata array with spi bytes
   {
                   //  spipasm_shiftin(Dpin, Cpin, Mode, Bits)
     pixydata[i] = spipasm_SHIFTIN(spia, 0, 2, 0, 8);  // Mode 0 = MSBPRE
     i++;
   }
   i = 1;
   while(i<198)    //  write pixydata array
   {
   printi("color = %d\n", pixydata[i]);
   i++;
   }
 spipasm_stop(spia);
 }

 // These are the actual functions

 //  ****************************************
 spia_t *spipasm_start(int CLKDLY, 
                          int CLKST)
 {
   spia_t *device;                                    // Declare spipasm pointer
   device = (void *) malloc(sizeof(spia_t));         // Allocate memory for it  
  
   device->ClockDelay = CLKDLY;
   device->ClockState = CLKST;
   if(device->cog == 0)
   {
     device->command = 0;
     extern int binary_SPIAsm_dat_start[];
     device->cog = 1 + cognew((void*)binary_SPIAsm_dat_start, (void*)device);
   }
   return device;                                       // Return pointer to structure
 }

 //  ************************************
 void spipasm_stop(spia_t *device)
 {
   if(device->cog > 0)                                  // If cog running
   {
     cogstop(device->cog - 1);                          // Shut down cog
     device->cog = 0;                                   // Clear cog varaible
     free(device);                                      // Release allocated memory
   }  
 }

 //  ************************************
 void spipasm_SHIFTOUT(spia_t *device, int DQ, int CLK, int MD, int BIT, int VLU)
 {
   device->Dpin = DQ;
   device->Cpin = CLK;
   device->Mode = MD;
   device->Bits = BIT;
   device->Value = VLU;
   spipasm_setcommand(spia,1, &device->Dpin);
 //  device->command = 1 << 16 + device->Dpin;
 //  while(device->command);
 } 
 
 //  ************************************
 int spipasm_SHIFTIN(spia_t *device,
                     int DQ, int CLK, int MD, int BIT)
 {
   device->Dpin = DQ;
   device->Cpin = CLK;
   device->Mode = MD;
   device->Bits = BIT;

   spipasm_setcommand(spia, 2, &device->Dpin); 
  //device->command = 1 << 16 + device->Dpin;
  //while(device->command);
 //
   while(device->Flag);
   return device->Value;
 }
  
 //  ************************************
   void spipasm_setcommand(spia_t *device, int cmd, int *argptr)
 {
   device->command = (int) argptr | (cmd << 16);
   while(device->command);
 }  
  

Andy Lindsay (Parallax)

I think the only variable that needs to be volatile is value. That's the only one that an outside process might change.

Jazzed' comment about filling multiple values was on my radar, but I have not checked if the PASM needs to be modified before that will be possible.

Our next task will be to test for runtime errors. There may well be mistakes in there that compile correctly, but result in memory or values being not quite perfect for the PASM code. Maybe it will run on the first try, but if not, I'm sure we'll be able to get it debugged.

I think it is safe to leave your code in this single file undocumented development version while getting the bugs out and making sure it functions well. Once we are past that, the next step would be to move the parts to the files and document them like in Jazzed' list, which is basically what you already did once in the Library Studies tutorial.

twm47099

Jazzed,
Thanks for the advice.

At this point the code is primitive. I wanted to use the steps in the "Library Studies" tutorial that started with a working application and developed a library from that. So my first step was to see if I could port the spin application to C. This spin program was a step more complex than the blinkpasm example. I was concerned about posting an unfinished work last night, but I felt I was starting to run around in circles due to my limited knowledge of pointers and how to use them (particularly after spending an hour tracking down a compile error that was just me not capitalizing part of a function name).

I was thinking about declaring the pins in the start function, but I wasn't sure if some SPI devices didn't use different I/O pins for different channels or something. The Pixy doesn't even use an input pin. I would expect the clock pin to always be the same for a given device. But since the spin model I was using included them in each shiftin or shiftout call I felt that I'd do the same. Also, at this stage I'm still trying to transition from crawling to walking, but once I get this running I will want to improve its efficiency and generality (maybe not in the same function).

I'm not even sure how I would go about using a buffer in this code without modifying the PASM. My clumsy use of the pixydata[] array in main() was an attempt to make sort of a buffer.

1. Use one loop to just collect a fixed number of bytes into an array - sufficient to collect two 20ms Frames of data,
2. Then have a subsequent loop that does something with the array -- in the example it just prints the values, in a pixy application it would have to find the 4 bytes that signal the start of a frame, then build the data words while checking for the start of a new block or frame).

I had made that modification in the original SPI_Asm_demo.spin and found that I was able to increase the number of Pixy blocks of data per Frame by a factor of 5 or more. (each block is the data for an object of a defined color or combination of colors)

Thanks for the advice on documentation. Usually, I do try to document the program before I write the code as an outline (particularly when I was writing forth, otherwise something I wrote at 3 AM would be a mystery when I woke up.) But I like the examples you gave. In this case I was using the spin program as the documentation and wanted to get the code and my questions posted.

Once I get the basic library working I will have some questions for you on how to optimize the code (in the Prop GCC or Prop1 forums).

Thanks again
Tom