 |
 |
| View previous topic :: View next topic |
| Author |
Message |
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
| PIC18F26J50 USB CDC issue |
 Posted: Wed Apr 09, 2014 1:49 am |
 |
|
Hi All,
I work on a device (PIC18F26J50) which is controlled by commands sent via USB CDC. The device is powered from USB, there is 1k5 resistor on D+ and it uses external crystal 12MHz. Please see the test program below. By application written in C# I am periodically sending the commands and read the answers. Everything works fine for quite long time (last time I tested it was 49 minutes) but sometimes communications fails (serial port timeout is occurred in C# app) – LED is still flashing.
I am not sure where the problem is. Could you please give me advice on what I have to focus?
(I am using CCS Compiler version 4.135)
Thanks in advance
Daniel
| Code: | #include <18F26J50.h>
#FUSES NOWDT
#FUSES DEBUG
#FUSES NOXINST
#FUSES NOSTVREN
#FUSES NOPROTECT
#FUSES NOFCMEN
#FUSES NOIESO
#FUSES NOCPUDIV
#FUSES HSPLL
#FUSES PLL3
#FUSES RTCOSC_INT
#USE fixed_io(b_outputs = PIN_B3)
#use delay(clock=48000000)
#define USB_EXTERNAL_PULLUPS
#define LED_ON {output_high( PIN_B3);}
#define LED_OFF {output_low( PIN_B3);}
#include <usb_cdc.h>
unsigned long cmd_counter = 0;
void init_hardware( void)
{
setup_oscillator(OSC_PLL_ON);
usb_init();
}
// Sending message via USB
void send_message(char *buff)
{
// Wait untill there is space in the transmit buffer
do { } while (usb_cdc_putready() != TRUE);
usb_cdc_puts(buff);
}
// Execution of command
void execute_cmd( char *command)
{
unsigned int32 key = 0;
int1 error = 0;
char output[20];
// convert cmd string to int32
int i;
for(i=0; i<3; i++)
{
if(command[i] != 0)
{
key |= (unsigned int32)command[i] << (16 - i * 8);
}
else
{
error = 1;
break;
}
}
if(error == 0)
{
// key == "CMD"
if(key == 0x434D44)
{
sprintf(output, "OK %Lu\r\n", cmd_counter);
cmd_counter++;
}
// key == "CLR"
else if(key == 0x434C52)
{
cmd_counter = 0;
sprintf(output, "OK\r\n");
}
// invalid command
else
{
sprintf(output, "Invalid command\r\n");
}
}
else
{
sprintf(output, "Invalid command\r\n");
}
send_message(output);
// clear output
memset(output, 0, sizeof(output));
}
void main( void)
{
int word;
char command[10] = {0};
int index = 0;
unsigned int32 LED_counter = 1000000;
int1 LED_state = 0;
init_hardware();
while (1)
{
if (usb_cdc_kbhit())
{
word = usb_cdc_getc();
if(word == 0x0D || word == 0x0A)
{
// string terminate
command[index] = 0;
// execute a command
execute_cmd(command);
// clear command and index
memset(command, 0, sizeof(command));
index = 0;
}
else
{
command[ index++] = word;
}
}
// Flashing LED
if (LED_counter == 0)
{
if (LED_state == 1)
{
LED_OFF;
LED_state = 0;
}
else
{
LED_ON;
LED_state = 1;
}
LED_counter = 1000000;
}
else
{
LED_counter--;
}
}
}
|
|
|
 |
alan
Joined: 12 Nov 2012
Posts: 357
Location: South Africa
|
|
| Posted: Wed Apr 09, 2014 1:58 am |
|
|
I don't have experience using USB but, maybe just sanitize the index var. If you miss the linefeed you will start overwriting your memory.
Regards
Alan |
|
|
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
|
| Posted: Wed Apr 09, 2014 3:12 am |
|
|
Hi Alan,
Thank for quick reply, you are right. But I don't think it is the major issue. By my C# application are sending just commands "CMD\r" and "CLR\r". Problem with overwriting memory should not occur.
Thanks
Daniel |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19554
|
|
| Posted: Wed Apr 09, 2014 8:20 am |
|
|
NOSTVREN, is unusual/dangerous. Means that you can get garbage operation if the stack does overflow. It shouldn't, but better to restart if it does....
I'd simply add a limit on index.
After incrementing it, have:
if (index==10) index=9;
Which will _ensure_ it cannot go beyond the end of the buffer.
Are you really using external pull-ups?. Seems pointless?. However you say you have a 1.5K pull up (presumably to Vusb - you don't say?), but make no mention of the pull down's. USB requires a 15K pull down on both D+ & D-. When external resistors are being supplied by you, you need these as well. Microchip carefully forget to tell you this, but it is in the USB spec, and a quick test shows that the pull down's are also disabled when you switch to using external resistors.....
I'd be looking very carefully at the smoothing on Vusb. Noise here can lead to major problems. How are you regulating the supply to the chip, and to Vusb?. Remember that Vbus on USB, can be up to 5v. The chip is rated for 3.6v max, and Vusb, needs to be between 3v, and 3.6v.
Now, you might get problems if the PC is trying to suspend the device. Try turning this off, and see if it changes anything. Control Panel, Hardware & sound, Power Options, Edit plan settings, Change advanced power settings, USB settings, USB selective suspend setting.
Best Wishes |
|
|
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
|
| Posted: Thu Apr 10, 2014 3:07 am |
|
|
Hi Ttelmah,
Thanks for your comment.
I removed the NOSTVREN from "FUSES" section.
Limit for index has been added.
Finally I removed the 1.5K resistor (yes, it was connected to Vusb) and currently I am using the internal resistors (#define USB_EXTERNAL_PULLUPS has been removed).
The device have a switch regulator (5V from USB -> 8V) and a linear regulator (8V -> 3.3V). On Vusb there is min 3.23V and max 3.29V. Also there is capacitor 100 nF connected on Vusb pin. From the linear regulator is powered also uC. So I think it should be OK.
I also tried disable suspend mode in setting for USB.
But everything without success, after some time of testing the communication timeout occurs anyway. It seems to me that when USB CDC communication fails, the PIC thinks that answer has been sent but PC is waiting for the answer without success.
I tried to look at the USB signal and signal from PIC seems little weird to me. I don’t know if it is normal? Please see the link.
https://drive.google.com/file/d/0BzNgfXLkvEwFeEMzcUNpQ01IS2s/edit?usp=sharing
Thanks
Daniel |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19554
|
|
| Posted: Thu Apr 10, 2014 6:15 am |
|
|
Are you saying you are running a boost converter to get 8v from 5v?.
If so, have you considered the power capabilities properly?. A boost converter can put nasty spikes back onto the power line. Have you configured the driver to specify that USB should deliver more current, or just left it as it is?. You are throwing away about 70% of the power you are drawing from the bus. How much current does the circuit draw?.
The spikes you show, are typical if the PCB layout doesn't give a good match to the bus. There are quite strict guidelines for the track layout on the board. Try putting 39R series resistors in the D+ and D- lines at the USB connector. |
|
|
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
|
| Posted: Mon Apr 14, 2014 2:52 am |
|
|
Hi Ttelmah,
Yes, there is more IC on the board. Max current which board can take is about 211 mA. I have added this definition into my code: “#define USB_CONFIG_BUS_POWER 300 //300mA (range is 0..500)”.
Yes, maybe I could try the resistors you mentioned if you think that it could fix the problem with USB communication.
About the communication issue, do you think it is rather hardware or software problem?
Thanks
Daniel |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19554
|
|
| Posted: Mon Apr 14, 2014 3:14 am |
|
|
There are several things that might probably be done better (the conversion to int32 for example, can be done with the make32 instruction, and would probably take about 1/100th the time!....), but you have the obvious 'real danger' ones, like you are correctly null terminating the string etc..
Have you added the index limit as suggested?. It really should be done.
Have you tested on more than one PC?. There are some oddities with various USB chipsets that can lead to unreliable operation (some embedded Intel ones in particular).
I really would be most suspicious of something like a power management setting. |
|
|
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
|
| Posted: Tue Apr 15, 2014 2:44 am |
|
|
Hi Ttelmah,
Yes, the index limit has been added. My actual code is below. My current code is below.
| Code: |
#include <18F26J50.h>
#FUSES NOWDT
#FUSES DEBUG
#FUSES NOXINST
#FUSES NOPROTECT
#FUSES NOFCMEN
#FUSES NOIESO
#FUSES NOCPUDIV
#FUSES HSPLL
#FUSES PLL3
#FUSES RTCOSC_INT
#USE fixed_io(b_outputs = PIN_B2, PIN_B3)
#use delay(clock=48000000)
#define CMD_length 20
//A5 old B2 new
#define SL_LED_ON {output_high( PIN_B2);}
#define SL_LED_OFF {output_low( PIN_B2);}
#define LED_ON {output_high( PIN_B3);}
#define LED_OFF {output_low( PIN_B3);}
#define USB_CONFIG_BUS_POWER 300 //300mA (range is 0..500)
#include <usb_cdc.h>
unsigned long cmd_counter = 0;
void init_hardware( void)
{
setup_oscillator(OSC_PLL_ON);
usb_init();
}
// Sending message via USB
void send_message(char *buff)
{
// Wait untill there is space in the transmit buffer
do { } while (usb_cdc_putready() != TRUE);
usb_cdc_puts(buff);
}
// Execution of command
void execute_cmd( char *command)
{
unsigned int32 key = 0;
char output[CMD_length];
// convert cmd string to int32
key = make32(command[0],command[1],command[2]);
// command execution
// key == "CMD"
if(key == 0x434D44)
{
sprintf(output, "OK %Lu\r\n", cmd_counter);
cmd_counter++;
}
// key == "CLR"
else if(key == 0x434C52)
{
cmd_counter = 0;
sprintf(output, "OK\r\n");
}
// invalid command
else
{
sprintf(output, "Invalid command\r\n");
}
send_message(output);
// clear output
memset(output, 0, sizeof(output));
}
void main( void)
{
char word;
char command[CMD_length] = {0};
int index = 0;
int1 usb_connected = 0;
unsigned int32 LED_counter = 1000000;
int1 LED_state = 0;
init_hardware();
while (1)
{
usb_task();
if (usb_enumerated())
{
if (usb_cdc_carrier.dte_present)
{
if (usb_connected == 0)
{
usb_connected = 1;
}
if (usb_cdc_kbhit())
{
word = usb_cdc_getc();
if(word == 0x0D || word == 0x0A)
{
// string terminate
command[index] = 0;
// execute a command
execute_cmd(command);
// clear command and index
memset(command, 0, sizeof(command));
index = 0;
}
else
{
if(index == CMD_length)
{
index = CMD_length - 1;
}
command[ index++] = word;
}
}
}
else
{
usb_connected = 0;
}
}
else
{
usb_connected = 0;
}
// SL_LED
if(usb_connected)
{
SL_LED_ON;
}
else
{
SL_LED_OFF;
}
// Flashing LED
if (LED_counter == 0)
{
if (LED_state == 1)
{
LED_OFF;
LED_state = 0;
}
else
{
LED_ON;
LED_state = 1;
}
LED_counter = 1000000;
}
else
{
LED_counter--;
}
}
}
|
Yes, I tried it on more PC and with similar result.
I tried to monitor USB communication with Free Device Monitoring Studio and to catch the serial port timeout issue. And I found out an interesting thing. It seems to me that PIC responds correctly but the answer isn’t delivered from USB layer to virtual serial port.
Thanks
Daniel |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19554
|
|
| Posted: Tue Apr 15, 2014 3:55 am |
|
|
| As written, it could still overshoot the buffer. The test needs to be after the increment, otherwise the index could be pointing past the end of the buffer when the null terminator is added.... |
|
|
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
|
| Posted: Tue Apr 15, 2014 5:07 am |
|
|
Hi Ttelmah,
Yes, you are right. Hope this version is correct.
| Code: |
#include <18F26J50.h>
#FUSES NOWDT
#FUSES DEBUG
#FUSES NOXINST
#FUSES NOPROTECT
#FUSES NOFCMEN
#FUSES NOIESO
#FUSES NOCPUDIV
#FUSES HSPLL
#FUSES PLL3
#FUSES RTCOSC_INT
#USE fixed_io(b_outputs = PIN_B2, PIN_B3)
#use delay(clock=48000000)
#define CMD_length 20
//A5 old B2 new
#define SL_LED_ON {output_high( PIN_B2);}
#define SL_LED_OFF {output_low( PIN_B2);}
#define LED_ON {output_high( PIN_B3);}
#define LED_OFF {output_low( PIN_B3);}
#define USB_CONFIG_BUS_POWER 300 //300mA (range is 0..500)
#include <usb_cdc.h>
unsigned long cmd_counter = 0;
void init_hardware( void)
{
setup_oscillator(OSC_PLL_ON);
usb_init();
}
// Sending message via USB
void send_message(char *buff)
{
// Wait untill there is space in the transmit buffer
do { } while (usb_cdc_putready() != TRUE);
usb_cdc_puts(buff);
}
// Execution of command
void execute_cmd( char *command)
{
unsigned int32 key = 0;
char output[CMD_length];
// convert cmd string to int32
key = make32(command[0],command[1],command[2]);
// command execution
// key == "CMD"
if(key == 0x434D44)
{
sprintf(output, "OK %Lu\r\n", cmd_counter);
cmd_counter++;
}
// key == "CLR"
else if(key == 0x434C52)
{
cmd_counter = 0;
sprintf(output, "OK\r\n");
}
// invalid command
else
{
sprintf(output, "Invalid command\r\n");
}
send_message(output);
// clear output
memset(output, 0, sizeof(output));
}
void main( void)
{
char word;
char command[CMD_length] = {0};
int index = 0;
int1 usb_connected = 0;
unsigned int32 LED_counter = 1000000;
int1 LED_state = 0;
init_hardware();
while (1)
{
usb_task();
if (usb_enumerated())
{
if (usb_cdc_carrier.dte_present)
{
if (usb_connected == 0)
{
usb_connected = 1;
}
if (usb_cdc_kbhit())
{
word = usb_cdc_getc();
if(word == 0x0D || word == 0x0A)
{
// string terminate
command[index] = 0;
// execute a command
execute_cmd(command);
// clear command and index
memset(command, 0, sizeof(command));
index = 0;
}
else
{
command[ index++] = word;
// index limit
if(index == CMD_length)
{
index = CMD_length - 1;
}
}
}
}
else
{
usb_connected = 0;
}
}
else
{
usb_connected = 0;
}
// SL_LED
if(usb_connected)
{
SL_LED_ON;
}
else
{
SL_LED_OFF;
}
// Flashing LED
if (LED_counter == 0)
{
if (LED_state == 1)
{
LED_OFF;
LED_state = 0;
}
else
{
LED_ON;
LED_state = 1;
}
LED_counter = 1000000;
}
else
{
LED_counter--;
}
}
}
|
Thanks
Daniel |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19554
|
|
| Posted: Tue Apr 15, 2014 7:44 am |
|
|
The critical question is whether anything improves!...
Have you considered the possibility that the problem might be something completely 'external'?. Remember a poster (might have been here), who had code that crashed at random intervals. Turned out it was when somebody used the photocopier just beside his bench, and it caused a momentary power spike....
Best Wishes |
|
|
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
|
| Posted: Fri Apr 18, 2014 1:41 am |
|
|
Hi Ttelmah,
The problem is same.
Hmm, I think there isn't anything in my area what would interfere with communication.
I found out other issue about usb_cdc communication. I have tried to add echo to my program and then I tested communication via Putty. It didn’t work as I expected. Mostly PIC didn't send answer but incrementation for CMD command had worked. Or new line isn't sent. I expected that communication will be as follow (I was sending just CMD commands):
CMD
OK 0
CMD
OK 1
CMD
OK 2
etc...
But it was for example like this:
OK 0
CMD
CMD
OK 2
OK 3
OK 4
CMD
CMD
OK 6
etc...
Is something not correct about my sending data via USB_CDC? I know that maybe I can use function usb_cdc_putc, but I think it should also work with usb_cdc_puts.
My program is here:
| Code: |
#include <18F26J50.h>
#FUSES NOWDT
#FUSES DEBUG
#FUSES NOXINST
#FUSES NOPROTECT
#FUSES NOFCMEN
#FUSES NOIESO
#FUSES NOCPUDIV
#FUSES HSPLL
#FUSES PLL3
#FUSES RTCOSC_INT
//A5 old B2 new
#USE fixed_io(a_outputs = PIN_A5)
#USE fixed_io(b_outputs = PIN_B3)
#use delay(clock=48000000)
#define CMD_length 20
//A5 old B2 new
#define SL_LED_ON {output_high( PIN_A5);}
#define SL_LED_OFF {output_low( PIN_A5);}
#define LED_ON {output_high( PIN_B3);}
#define LED_OFF {output_low( PIN_B3);}
#define USB_CONFIG_BUS_POWER 300 //300mA (range is 0..500)
#include <usb_cdc.h>
unsigned long cmd_counter = 0;
void init_hardware( void)
{
setup_oscillator(OSC_PLL_ON);
usb_init();
}
// Sending message via USB
void send_message(char *buff)
{
// Wait untill there is space in the transmit buffer
do { } while (usb_cdc_putready() != TRUE);
usb_cdc_puts(buff);
}
// Execution of command
void execute_cmd( char *command)
{
unsigned int32 key = 0;
char output[CMD_length];
// convert cmd string to int32
key = make32(command[0],command[1],command[2]);
// command execution
// key == "CMD"
if(key == 0x434D44)
{
sprintf(output, "OK %Lu\r\n", cmd_counter);
cmd_counter++;
}
// key == "CLR"
else if(key == 0x434C52)
{
cmd_counter = 0;
sprintf(output, "OK\r\n");
}
// invalid command
else
{
sprintf(output, "Invalid command\r\n");
}
send_message(output);
// clear output
memset(output, 0, sizeof(output));
}
void main( void)
{
char word;
char command[CMD_length] = {0};
int index = 0;
char buff[CMD_length] = {0};
int1 usb_connected = 0;
unsigned int32 LED_counter = 1000000;
int1 LED_state = 0;
init_hardware();
while (1)
{
usb_task();
if (usb_enumerated())
{
if (usb_cdc_carrier.dte_present)
{
if (usb_connected == 0)
{
usb_connected = 1;
}
if (usb_cdc_kbhit())
{
word = usb_cdc_getc();
// echo - print character
sprintf(buff, "%c", word);
send_message( buff);
if(word == 0x0D || word == 0x0A)
{
// Echo - print new line
sprintf(buff, "\r\n");
send_message( buff);
// string terminate
command[index] = 0;
// execute a command
execute_cmd(command);
// clear buff, command and index
memset(buff, 0, sizeof(buff));
memset(command, 0, sizeof(command));
index = 0;
}
else
{
command[ index++] = word;
// index limit
if(index == CMD_length)
{
index = CMD_length - 1;
}
}
}
}
else
{
usb_connected = 0;
}
}
else
{
usb_connected = 0;
}
// SL_LED
if(usb_connected)
{
SL_LED_ON;
}
else
{
SL_LED_OFF;
}
// Flashing LED
if (LED_counter == 0)
{
if (LED_state == 1)
{
LED_OFF;
LED_state = 0;
}
else
{
LED_ON;
LED_state = 1;
}
LED_counter = 1000000;
}
else
{
LED_counter--;
}
}
}
|
Thanks
Daniel |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19554
|
|
| Posted: Fri Apr 18, 2014 1:55 am |
|
|
Yes there is a problem with what you are doing....
putready, does not return a true/false. It returns how much space is in the buffer.
You don't actually need this test at all, unless you want to go and do something else while waiting for the usb buffer to clear, but the correct code is:
| Code: |
void send_message(char *buff)
{
//now have how big the message is
// Wait until the buffer is empty
do { } while (usb_cdc_putempty() != TRUE);
usb_cdc_puts(buff);
}
// or use strlen to read the length of 'buff' and wait for putready to
//be larger than this.
|
|
|
|
Donald_X
Joined: 08 Apr 2014
Posts: 9
|
|
| Posted: Fri Apr 18, 2014 2:31 am |
|
|
Ttelmah
I have probably the older version of CCS compiler. There isn't function usb_cdc_putempty. In my case the usb_cdc_putready should returns TRUE if there is room left in the transmit buffer for another character. But you are right it not help me with usc_cdc_puts at all.
Hmm, probably I should find some other workaround to find out that buffer is empty. Maybe this could help:
| Code: |
void send_message(char *buff)
{
// In case buffer is busy try to send again
while(usb_cdc_puts(buff) != TRUE);
}
|
Thanks
Daniel |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
