 |
 |
| View previous topic :: View next topic |
| Author |
Message |
lecutus
Joined: 12 Aug 2009
Posts: 40
|
| Why doesn't this work? |
 Posted: Mon Sep 28, 2009 1:44 pm |
 |
|
This is going to be one of those “simple answer things” that I can't see.
Why doesn't this work? I am reading a string in off of rs232, its coming in because I can see it, on the putc(c) to the screen. Now within the same block of code, the next line places 'c', the variable array. At the end when all the arrays are supposedly filled, then the printf and nothing.
The string: !!!5000;750;125;10000;2&&&"
Problem code:
| Code: | void get_info_rs232()
{
/*get info in , parse, put to appropriate areas */
char c;
int8 strLn = 0;
char id[5],r[5],g[5],op[6],sw[5];
int8 va;
//printf(" inside get_ifo_rs232"); line_feed();
//printf("srt1=%s and is %u long", str1, strlen(str1)); line_feed();
While (c != 38)
{
c = getc();
If (c != 33)
/*{
}
else*/
{
if (isdigit(c))
{
while(isdigit(c))
{
putc(c);
id[strLn++]= c;
c = getc();
}
id[strLn++]= 0; // null termination
c = getc();
strLn = 0;
}
if (isdigit(c))
{
while(isdigit(c))
{
putc(c);
r[strLn++]= c;
c = getc();
}
r[strLn++]= 0; // null termination
c = getc();
strLn = 0;
}
if (isdigit(c))
{
while(isdigit(c))
{
putc(c);
g[strLn++]= c;
c = getc();
}
g[strLn++]= 0; // null termination
c = getc();
strLn = 0;
}
if (isdigit(c))
{
while(isdigit(c))
{
putc(c);
op[strLn++]= c;
c = getc();
}
op[strLn++]= 0; // null termination
c = getc();
strLn = 0;
}
if (isdigit(c))
{
while(isdigit(c))
{
putc(c);
sw[strLn++]= c;
c = getc();
}
sw[strLn++]= 0; // null termination
c = getc();
strLn = 0;
}
}
//printf("ID=%s RPM=%s Gear=%s OP=%s SWI=%s \r\n",id, r,g,op,sw);
pump_id=atol(id); // original place
rpm_rs=atol(r); //convert string to number
gear_rs=atoi(g);
ov_press_rs=atol(op);
swi_rs=atoi(sw);
// printf("ID=%lu RPM=%lu Gear=%u OP=%lu SWI=%u \r\n",pump_id, rpm_rs,gear_rs,ov_press_rs,swi_rs);
}
//printf(" outing get_ifo_rs232"); line_feed();
}
|
PCWHD 4.079
L.
Last edited by lecutus on Mon Sep 28, 2009 3:54 pm; edited 1 time in total |
|
 |
Ttelmah
Guest
|
|
| Posted: Mon Sep 28, 2009 2:52 pm |
|
|
There are several little things that might cause problems:
First, you don't initialise 'c' before the first test. It may contain a value from a previous pass, or a value depending on the memory contents...
Then all the printf's at the end are remmed out. So you won't get anything from them....
Then the code will exit at the first '&', so the next time through, there will be another '&' waiting to arrive.
Do some significant tidying, perhaps something like:
| Code: |
char get_first_digit(void) {
int8 c;
do {
c=getc();
} while (!(isdigit(c));
//Loop till first digit is seen
return c; //Exit with the digit found
}
int16 get_number(char *array, int8 max) {
int8 c;
int8 ctr=0;
//Find first numeric digit
c=get_first_digit();
//Arrive with digit found, store string into array, for 'max'
//characters at most
do {
array[ctr++]=c;
c=getc();
} while (isdigit(c) && ctr<max);
if (ctr<max) array[ctr]='\0';
else array[ctr-1]='\0'; //still terminate if overflow
}
void get_info_rs232(void) {
char id[5],r[5],g[5],op[6],sw[5];
char tempc;
get_number(id, 5);
get_number(r, 5);
get_number(g, 5);
get_number(op, 6);
get_number(sw, 5);
tempc=getc();
tempc=getc(); //dump trailing '&' signs
printf("ID=%s RPM=%s Gear=%s OP=%s SWI=%s \r\n",id, \
r,g,op,sw);
//Add retrieval code here You can just use atol throughout,
//provided 'op', can never exceed 65536.
}
|
Best Wishes |
|
|
barryg
Joined: 04 Dec 2006
Posts: 41
|
|
| Posted: Mon Sep 28, 2009 3:03 pm |
|
|
| Quote: | | This is going to be one of those “simple answer things” that I can't see. |
Nothing prints out, because you commented out all the printf's 
As for the "I can't see" part? An editor that colors the syntax (comments turn green) is a wonderful thing. |
|
|
lecutus
Joined: 12 Aug 2009
Posts: 40
|
| Clarification |
| Posted: Mon Sep 28, 2009 3:25 pm |
|
|
The printf's at the end are checking the individual arrays for information, those are turned on and off to check the behaviour. They will not be there at the end of development.
The input string is sent every 250 millisecs. The '&' is used as a marker that says, "the end".
The problem still remains the individual bytes is not being placed into the array. But the individual bytes are displayed via putc. The parsing is correct.
What I don't see is how my
| Code: |
while(isdigit(c))
{
putc(c);
id[strLn++]= c;
c = getc();
}
|
is different from Ttelmah's. Nice coding by the way.
| Code: |
do {
array[ctr++]=c;
c=getc();
} while (isdigit(c) && ctr<max);
|
They both still use getc and an array.
I did initialized 'c' and still just gibberish. So what am I missing?
L. |
|
|
asmboy
Joined: 20 Nov 2007
Posts: 2128
Location: albany ny
|
|
| Posted: Mon Sep 28, 2009 8:29 pm |
|
|
one obvious problem with your version can be found by answering the following questions:
1- what happens if c is not a digit when your while loop is ( attempting to be ) entered initially ?
2- what might the value of c actually BE ,
that you test before reading a new value of c INSIDE the loop ?
perhaps this will open your thinking up a bit
- the phrase
'cart before horse' ? |
|
|
lecutus
Joined: 12 Aug 2009
Posts: 40
|
|
| Posted: Tue Sep 29, 2009 8:48 am |
|
|
The algorithm works, the logs show that the string is taken and stripped properly of all extrenoius, nonnumerical characters. If you'll look at the packet format vs. the initial start, all paramters are present. Changing the rpm the gear, and the over_pressure occurred as desired. So the information is getting in and parsed properly
Basic premise:
Check for '!', if yes, continue with the parse, else wait for next incoming .
Parsing:
Is char a digit, if no, move to next char, else add to holding array until a ';' is encountered and move to next holding array. Continue until '&' is encountered and stop. Wait for next string. Next string every 250 millisec.
Packet format looks like this:
!!!5000 ;2000;125;7400;0&&&
!!!TRUCK_ID ;rpm;gear;over_pressure_setpt;switch_status&&&
initial start:
| Quote: | 2009/09/29 08:24:59:5937 PIC5000 started
2009/09/29 08:24:59:8906 5000 's port_in has
50007401245000
2009/09/29 08:25:00:3750 5000 's port_in has
5
2009/09/29 08:25:00:8593 5000 's port_in has
50007401245000
2009/09/29 08:25:01:3750 5000 's port_in has
5
2009/09/29 08:25:02:8750 5000 's port_in has
50007401245000 |
After changing some paramters, algorithm still works:
| Quote: |
2009/09/29 08:26:42:3750 5000 's port_in has
500090012514000
2009/09/29 08:26:42:8750 5000 's port_in has
5
2009/09/29 08:26:44:3750 5000 's port_in has
500090012514000
|
Preprocessor:
| Code: | #include <18F258.h>
//#fuses HS,PROTECT,NOLVP,WDT32,nowdt// original
#fuses HS,PROTECT,NOLVP,nowdt,brownout,BORV42,noprotect
#use delay(clock=20000000)
#use rs232(baud=115200, xmit=PIN_C6, rcv=PIN_C7, stream=rs_str) // Jumpers: 8 to 11, 7 to 12
#include <can-18xxx8.c>
#byte pir1=0x0F9E
#define TMR1iF 0X00
#rom int 0xf00000={0x00,0x00,0x00,0x00}
#define offset_id 0x00
#define offset_e 0x02// original
#define offset_t 0x03// original
#include <string.h>
#include <stdlib.h>
#include <stdio.h>
#include <input.c>
|
The information is confirmed to be there by the fact that the putc(c) puts the character the screen/log, so why doesn't it put it into the next cell in the array.
The main question again and still remains: Why doesn't this put the characters into the said array?
| Code: | putc(c);
r[strLn++]= c;
|
PCWHD 4.079
Let's stay focused
L. |
|
|
lecutus
Joined: 12 Aug 2009
Posts: 40
|
| Clarification on the technique |
| Posted: Tue Sep 29, 2009 3:15 pm |
|
|
The question to Ttelmah specifically and to anyone in general. I've never seen this particular use of void before. What is the significants of the void after the name of the function and the double use of void in the second function.
| Ttelmah wrote: |
| Code: |
char get_first_digit(void) {}
void get_info_rs232(void) {}
|
|
What are the advantages.
Please enlighten!!!!
L. |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
|
lecutus
Joined: 12 Aug 2009
Posts: 40
|
|
| Posted: Tue Sep 29, 2009 3:44 pm |
|
|
thanks
I've read the link, so in essence there's no difference or advantage? |
|
|
asmboy
Joined: 20 Nov 2007
Posts: 2128
Location: albany ny
|
| nitty gritty |
| Posted: Tue Sep 29, 2009 3:57 pm |
|
|
if you flat out VERBATIM copy TTelmah's code example - w/o putting the putc echo stuff in ,,, DOES HIS EXAMPLE, as posted , WORK?
and if NOT - what DOES appear in the recovered strings
you have GOT to simplify to the point where it starts to work, to see why it doesn't so far .
TTelmahs example looks bulletproof to me - so i would start there.
BTW: 115200 is a zippy baud and will show about 1.5% error absolute , though I do not think that is your issue. |
|
|
lecutus
Joined: 12 Aug 2009
Posts: 40
|
| Attempted here's the report |
| Posted: Wed Sep 30, 2009 1:18 pm |
|
|
ASM asks does it work, YES!!!!!!!!!!!!!!!!! Everything is as it should be, the initial string at start up, the updating works, gears change, it pressures up. Yep, it's all there.
When compiling I'm getting:
warning 208 Function not void and doesn't return a value get_number
As to the error percentage, I can live with it.
I'm still a little perplexed by the style of the use of the double void.
| Code: | | void get_info_rs232(void) |
I'll look it up later.
Now I'm wondering why my version didn't work, I'll do the post mortem on that later.
To expand my knowledge in the most critical sense, I like to understand why things work and why they don't. Because the way we do things in this shop, once done, do it again. In other words I need to get my string manipulation down.
I like to thank Ttelmah and everyone else.
Hey Ttelmah, take the rest of the week off.
Thanks
L.  |
|
|
Ttelmah
Guest
|
|
| Posted: Wed Sep 30, 2009 2:58 pm |
|
|
You would do.
I must admit, I was going to add the atol function into the get_number routine, so it only needs to appear once.
If you do this, you can actually save space, getting rid of the external string storage as well, just having a local string buffer big enough for the 'worst case' (six characters), then remove the string buffer address from the call, and declaration, and just call atol in the function, and return the 16bit value. atol, doesn't mind being called with a value smaller than a 'long', in the string.
So:
| Code: |
int16 get_number( int8 max) {
int8 c;
int8 ctr=0;
char array[6];
//Find first numeric digit
c=get_first_digit();
//Arrive with digit found, store string into array, for 'max'
//characters at most
do {
array[ctr++]=c;
c=getc();
} while (isdigit(c) && ctr<max);
if (ctr<max) array[ctr]='\0';
else array[ctr-1]='\0'; //still terminate if overflow
return(atol(array));
}
|
I removed this, since I wasn't sure if you did actually want the separate strings as well, and left the int16 declaration (slap slap.....).
Some languages, are really strongly typed, _requiring_ you to declare everything for types. I'm used to these, so tend to never leave type declarations 'blank', inserting 'void' if nothing is passed.
Best Wishes |
|
|
asmboy
Joined: 20 Nov 2007
Posts: 2128
Location: albany ny
|
|
| Posted: Wed Sep 30, 2009 4:28 pm |
|
|
The warning - it comes about because while the func tells the compiler it will return a 16 bit int, but the code does not RETURN a value when executed. No biggie
mostly harmless |
|
|
|
|
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
