Yes there are errors relating to the USTAT differences for the PIC24 in various places in the stack. I have previously posted in this thread some further details but I do not claim that I have found and related all of them.
I cannot find any reference anywhere as to why the COSC is set to FRCDIV at this point. I am still looking per chance that I missed something. However the data sheet and the oscillator reference manual don't seem to be shedding any light. Nor is there anything in the latest (2.8) microchip stack that alludes to anything being required. It may well be that the COSC bits are N/A as we select HS_PLL as a start-up oscillator and do not enable clock switch over. I am not sure.
While you are looking at that also check the USB_USTAT2BD(X) for the PIC24. They are the same for the PIC18 in the stack version I have here but that cannot be correct as the USTAT SPRs are different on each.
Sorry to be so brief right now I have other duties.
[quote author="rebeccapalmer"][ To define PIC_24F or PIC_18F for every processor I use: [code]#if defined __PIC24F__ || defined __PIC24FK__ //we don't care about 24H/30/33 because they don't have USB anyway [/quote]
Best I can tell none of the PIC24FK series have USB either.
[quote author="rebeccapalmer"]After further thought, I decided that rather than having to separately set USB_BDT_ADDRESS, USB_DATA_ADDRESS, USB_CDCDATA_ADDRESS (and more if we later add other device classes; #define USB_DATA_ADDRESS USB_BDT_ADDRESS+8*USB_NUM_ENDPOINTS doesn't work because arithmetic is not done inside #pragma directives), it made more sense to have one section for everything that needs to be in USB memory: only one USB_BDT_ADDRESS that must be set for each processor, no linker script required.
/* Allocate buffers for buffer description table and the actual buffers */ #if USB_PP_BUF_MODE == 0
#if defined(PIC_18F) #pragma udata usb_bdt=USB_BDT_ADDRESS volatile BDentry usb_bdt[2*USB_NUM_ENDPOINTS]; #elif defined(PIC_24F) volatile BDentry usb_bdt[2*USB_NUM_ENDPOINTS] __attribute__((aligned(512))); #endif /*If you get the linker error "section 'usb_bdt' can not fit the absolute section", * your buffers are too large for one 256-byte bank. Use additional banks * (if your device has them) with #pragma udata usb_data1=USB_DATA1_ADDRESS, etc
/* Only claim buffer for ep 0 */ unsigned char usb_ep0_out_buf[USB_EP0_BUFFER_SIZE]; unsigned char usb_ep0_in_buf[USB_EP0_BUFFER_SIZE]; #ifdef CDC_BUFFER_SIZE //are we a CDC class device? unsigned char cdc_acm_out_buffer[CDC_BUFFER_SIZE]; unsigned char cdc_acm_in_buffer[CDC_BUFFER_SIZE]; unsigned char cdc_rx_buffer[CDC_BUFFER_SIZE]; unsigned char cdc_tx_buffer[CDC_BUFFER_SIZE]; #endif #else #error "Ping pong buffer not implemented yet!" #endif
If there is too much for one bank (over 256 bytes; we currently use 72 bytes) it needs to be manually divided into banks with #pragma udata usb_data1=USB_DATA1_ADDRESS (0x500 on an 18F4550) etc (not doing so will cause a linker error), but this is also true of the linker script method. [/quote]
For certain something like this should be used. There is no point juggling linker scripts when there is no need to. Microchip have a system like this in their stack.
[quote author="rebeccapalmer"] [quote author="JTR"]As you can see in the last two there is no code generated for the runtime variable trn_status while there is for my proposed correction. ???[/quote] If ping-pong is off (the default), trn_status isn't supposed to do anything at this point, hence its absence from the compiled code. [quote author="JTR"]The runtime variable isn't just inside an array index, it is also inside a compile time macro.[/quote] Macros in C are text substitutions, not compile-time calculations as such (constant folding, where calculations that can be done at compile time are, is a separate process and occurs whether or not the constants are part of a macro), so the parameters can be run-time variables.[/quote]
Yes, thanks... I have worked it all out. I see how the C macros are work now. Funny thing is that I learn a lot about C on code that is not even required or used. :)
I noticed that in my debugging, the interrupt never fires. [/quote]
Oh but it did, just the wrong interrupt. :) EDIT: Vector that is.
[quote author="ian"]
It looks like there are a lot of good updates and findings. I can apply any patches and fixes mentioned in this thread to SVN. Is there a general consensus about which should be applied now? [/quote]
I have taken the view that there needs to be a reshuffle of how and where things are defined in the header files and even what header files are required. The stack I am working on is bit of a departure from the way it is organized now. My intent is to apply myself to getting this in order and then I will send you a zip file.
Where or not people agree with me doesn't really matter. I will not be offended if people see things differently or have better ideas. You don't have to use anything I have changed. However, I do have the experience of having done this before and knowing what works and why. When I have something tested and operational I will hand it over and explain the rational for the reorganization.
I am noting the comments posted here and if there is any correction or good idea then I will incorporate that into my work.
Meantime any further work done on the CDC side of things can easily be added to what I have done.
As you can see in the last two there is no code generated for the runtime variable trn_status while there is for my proposed correction. ???
The runtime variable isn't just inside an array index, it is also inside a compile time macro.
However! As rebecca suggests I have to agree that bd is not used at all and the whole thing is moot. Looks to me that the code can be removed. It does nothing. I wished I had of looked closer at that. Would of saved an hours work. :(
Otherwise, well as it appears to me, you are trying to XOR a compile time variable to unknown run time variable to create the index to the usb_bdt.
I looked at the dissassembly and the code for the first case just is not doing anything like what is required.
Right now, because we are not using ping-pong buffering it does not make a difference to the operation of the code. However if ping-pong is added it surely will.
As it is, even with my correction, the code is not correct for the PIC24 parts and will need to be:
A very timely post Rebecca! I am just looking at all this now and formulating the best, most flexible approach. I have already done all this with my assembler stack and have a good template to work with.
I missed the shorthand way of determining a PIC24. Way to go!
Yes the issue of the number of endpoints needs to be addressed in a few places. Already I have done something similar to what you did however I labeled the max number of endpoint on the chip as MAX_CHIP_ENDPOINT as USB_MAX_ENDPOINTS is already used in a different way in several other existing stacks.
Re the configuation bits and hardware init. This is not really an issue. These belong not to the stack but to the hardware. They are implementation specific and except for the PLL settings they are always going to be highly variable and user defined.
Yeah, there are a few holes in the stack like this. The header files need to be reorganized somewhat so that a project written say for the 18F24J50 with very easily port to say the 18F47J53. This should be a cinch with no hidden gotchas hiding deep within the stack proper or other unintuitive or undocumented headers.
Near on 6AM here in Oz and I have just finished a 12 hour straight debug session. Happy to say that I got the basic stack working with all the changes. It turned out to be a big job. How, what, why and when the stack worked previously is something I am puzzled by as I found a real show stopper with the interrupts. Basically priority interrupts where being used but I could not find anywhere in the code how they were enabled. By default the legacy mode is enabled and this was not changed by setting bit-7 in the RCON register.
Re the PIC24 support. There are some bit differences from the PIC18 pics that have not been addressed. Please double check these corrections in the usb_p24f.h header.
While I an waiting on my hardware to arrive I should be able to zip up the corrections I have made and so we can see how they go. I have addressed a few PIC24 specific bugs and also brought my USB knowledge to address a few TODO things that were marked with a "?" and some other errors that trace back to the original source this effort was inspired by.
Design wise, I stayed with the principle of keeping the core USB stack (I.E. CHP9 specs and house keeping) in the one module. Already the stack was pretty much like this and it that is well done. You will see the wisdom of this when you discover how easy it will be in future to use the stack for a HID keyboard and anything else you want. The core of the stack does not need to be altered. and can even be a object file that you simply link in.
I am at the point where I will start testing the updates on a PIC18F14K50 and a PIC18F2550. Other bits and pieces are in the mail.
After a day of poking around C documentation and trying things I have managed to bring myself further up to speed with programming in C and have been able to alter the code in the way I know well it needed to be. I didn't particularly want to do this as I felt my expertise was better directed at the function of the USB stack and not trying to learn C on the fly. However, in the long run it was probably better that I took that extra step.
Hope to give the stack a good hit out over the weekend.
Yeah, this debate stemming from the well constructed ambiguity of the data sheets has been debated on the microchip forums. It is very ambiguous.
Anyway. Back to the real task at hand. I need HELP! I am not pretending to be a C coder and there are somethings that need to be fixed up that is out of my league. What is required immediately are:
1) Either global variables or a macro/function that conveys the current USB CONFIGURED STATE. That is we need to be able to test the USB CONFIGURED STATE from within main.c. It will also be required in cdc.c
case USB_REQUEST_SET_CONFIGURATION: if (USB_NUM_CONFIGURATIONS >= packet[USB_wValue]) { // TODO: Support multiple configurations /* Configure endpoints (USB_UEPn - registers) */ usb_configured = packet[USB_wValue]; if (usb_configured !=0) // JTR addition { user_configured_init(); //JTR addtion, USB Function endpoints set up done via CALLBACK . // Note I have this working but what is preferred is for the callback to go to a separate device function module and not be handled in the CHP9 module. } rbdp->BDCNT = 0; rbdp->BDSTAT = UOWN + DTS + DTSEN; DPRINTF("DEV Set_Config 0x%02Xn", packet[USB_wValue]); } else usb_RequestError(); break;
In my assembler stack I just define global variables and use the extern directive etc. However as this is a C program there is a C way of doing things and the last thing I want to do is hack the code in a most unlike C way.
Without these two additions the stack is will be locked into a shaky path and not be very workable in the long run.
So is there any skilled C coder who can help out here. Of course you realize that it requires some resolution on what you want the stack to be. If you just want a CDC stack then why not stick everything into one file. If you want something flexible then judicious organization is required starting now. IMHO that is where the stack development is at. Currently it does not even test for the CONFIGURED STATE before trying to perform CDC functions. Also, if say you wanted to create a generic bulk device again you would have to alter the CHP9 code. This is not a desirable thing from my point of view.
The bottom line I have come to is that even if I can find and fix the logical errors (and I have found some) the fix-ups cannot really be tested until there is a workable way of testing for the CONFIGURED STATE. I do not plan to try to fudge the C code in this way. Someone, anyone?
@ rsdio. Yes, you are right. The internal oscillator for the "GB" series is not really good for USB. The "DA" series does however spec the internal oscillator for USB and this is where I got confused. It is interesting to note though that both series have the same maximum and minimum tolerances. ???
Note to all: I am adding a whole heap of new defines and header info to the existing stack. My aim is to have it portable across ALL PIC18 and PIC24 USB devices. That is - ready to work out of the box and not have to be rewritten to add device support. All device/hardware/project support can be defined in a single header file and the core of the stack will remain unchanged.
There are some corrections to the PIC24 support to come. Do not expect it to work for the PIC24 as is.
There will be some heavy lifting required to mature the stack. This is going to need some proficient C coders and that counts me out. However I can show what needs to be added and where and why as I have done all this in assembler.
