 |
 |
| View previous topic :: View next topic |
| Author |
Message |
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
 Posted: Sat Sep 03, 2016 3:32 pm |
 |
|
Thank you for the answers ckielstra and asmboy
asmboy, the rx interrupt is disabled and I outcommented the interrupt rutine
I enabled it just to test what I learned from PCM programmer.
ckielstra, the reason I have posted the program is because I wanted to learn how to make the program
| Quote: | | My question is if this work can be done in a more efficient and elegant way in C? |
I learned a few things already, but not yet how to make the program shorter and more efficient. I know:
| Quote: | | code has a lot of duplication which makes your program very long and difficult to maintain |
Best wishes
Joe |
|
 |
temtronic
Joined: 01 Jul 2010
Posts: 9632
Location: Greensville,Ontario
|
|
| Posted: Sat Sep 03, 2016 4:40 pm |
|
|
In order to make any program efficient, you really need to understand the PIC's instruction set, print the 'listing' files and 'play PIC'. Knowing how the PIC does things goes a long way to coding in any high level language,be it Basic, C, Fortran, etc.
'Efficient' code is either broken down into two thoughts. 1) minimal code space or 2) execution speed. The former ( tight code) is for projects that are cost sensitive on a per unit basis. The latter (speedy), allows the PIC to do more in a given time frame. These days with 64MHz PICs and tons of memory ,neither is really necessary in 99% of the Real World applications.
There's 3 ways of coding....mine, yours and the other guys ! As long as it works up to customer's expectations it's fine.
In the 'good old days' when memory was very expensive(mag core) you HAD to KNOW how computers actually worked bit by bit....
Jay |
|
|
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
| Posted: Sat Sep 03, 2016 6:18 pm |
|
|
I know Jay
Started with assembler with 68HC811, moved to 16C84 and 16C71 because they relative high speed (4*clock=PC) and the simplicity of PIC assembler.
Has some Basic experience also.
I am 10 years (not continuously) with CCS and still have many things I don't know how to do in C 
This is the reason I started this topic, to see if have a way to make things simpler and better.
Best wishes
joe |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 20059
|
|
| Posted: Sun Sep 04, 2016 1:08 am |
|
|
The point is that efficiency, goes directly to your assembler knowledge.
For instance, if you declare the output latch (in CCS, use #BYTE LATB=getenv("SFR:LATB") )
Then all 16 of your possible outputs, can simply be done with:
| Code: |
LATB= (LATB & 0xF0) | (digitaldatacnt % 0xF);
|
No need for cases branches etc..
Think about what you are doing. You output the bottom 3 bits of digitaldatacnt, to select a bit number on the multiplexer, and then the next bit to select which of the two multiplexers to use (effectively you have a one of 16 multiplexer).
All this line does is read the whole data output latch, saves the top four bits of this, then combines this with the low four bits from your counter.
This one line replaces 64 lines of your current code.
Then, since your variables used to say whether a button is on/off, only hold logic values, I repeat my question. Why not use an array of int1 values?. Saves 14 bytes of RAM, and makes the code much easier as well. |
|
|
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
| Posted: Sun Sep 04, 2016 4:38 am |
|
|
Hi Ttelmah
I changed the data00...data15 as ckielstra advised, added what you proposed, I just don't know how to use know the data.
While compiling I am getting a warning:
| Quote: | | Variable never used: data |
I know is because I am not reading the data
| Code: | ///////////////////////////////////////////////////////////////////
//test program for reading 16 digital inputs///////////////////////
///////////////////////////////////////////////////////////////////
#include <18F26K22.h>
#device ADC=10
#FUSES NOWDT //No Watch Dog Timer
#FUSES INTRC_IO //
#FUSES PLLEN
#FUSES NOFCMEN //Fail-safe clock monitor disabled
#FUSES NOIESO //Internal External Switch Over mode disabled
#FUSES PUT //Power Up Timer
#FUSES BROWNOUT
#FUSES BORV29 //Brownout reset at 2.85V
#FUSES NOPBADEN //PORTB pins are configured as digital I/O on RESET
#FUSES NOHFOFST //High Frequency INTRC waits until stable before clocking CPU
#FUSES NOLVP //No low voltage prgming, B3(PIC16) or B5(PIC18) used for I/O
#FUSES NOXINST //Extended set extension and Indexed Addressing mode disabled (Legacy mode)
#use delay(internal=64MHz)
#use rs232(baud=9600, UART1, stream=PORT1, errors)
#ZERO_RAM
#define diginsltA PIN_B0
#define diginsltB PIN_B1
#define diginsltC PIN_B2
#define diginsltOE PIN_B3//active L for data0; H for data1 via inverter
#define diginDATA PIN_B4
#define flashled PIN_C3
#define txflashled PIN_A4
int flashledcnt=0;
int digitaldatacnt=0;
//int data00,int data01,data02,data03,data04,data05,data06,data07;
//int data08,data09,data10,data11,data12,data13,data14,data15;
#BYTE LATB=getenv("SFR:LATB")
//ckielstra
#define NUMBER_OF_INPUTS 16
int8 data[NUMBER_OF_INPUTS];
int tx1words=0;
int tx1dig0=0;
int tx1dig1=0;
int tx1wch=0;
int tx1timecnat=0;
//int rxdata=0;
//#include "T22DG.h"
///////////////////////////////////////////////////////////////////
#INT_TIMER3
void TIMER3_isr(void)
{
flashledcnt++;
tx1timecnat++;
if(flashledcnt==16)
{
output_toggle(flashled);//every 524ms
flashledcnt=0;
}
if(tx1timecnat==64)
{
enable_interrupts(INT_TBE);//every 2.096 seconds
output_toggle(txflashled);
tx1timecnat=0;
}
}
///////////////////////////////////////////////////////////////////
#INT_TBE
void TBE_isr(void)
{
switch(tx1words)
{
case 0:
{
fputc(85,PORT1);
tx1words++;
}
break;
case 1:
{
fputc(170,PORT1);
tx1wch=255;
tx1words++;
}
break;
case 2:
{
fputc(tx1dig0,PORT1);
tx1wch=tx1wch+tx1dig0;
tx1words++;
}
break;
case 3:
{
fputc(tx1dig1,PORT1);
tx1wch=tx1wch+tx1dig1;
tx1words++;
}
break;
case 4:
{
fputc(tx1wch,PORT1);
tx1words=0;
disable_interrupts(INT_TBE);
}
break;
}
}
///////////////////////////////////////////////////////////////////
/*
#int_RDA
void RDA_isr(void)
{
rxdata=getc();
}
*/
///////////////////////////////////////////////////////////////////
void main()
{
port_b_pullups(0xFF);
setup_timer_3(T3_INTERNAL | T3_DIV_BY_8);//32.75 ms overflow
enable_interrupts(INT_TIMER3);
disable_interrupts(INT_RDA);
disable_interrupts(INT_TBE);
enable_interrupts(GLOBAL);
while(TRUE)
{
delay_us(10);
LATB= (LATB & 0xF0) | (digitaldatacnt % 0xF);
// DG();
}
}
|
Can you tell me how to do that?
Best wishes
Joe |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 20059
|
|
| Posted: Sun Sep 04, 2016 5:31 am |
|
|
Your DG function, counts from 0 to 15, outputting this count in turn and reading the reply:
| Code: |
void DG(void)
{
LATB= (LATB & 0xF0) | (digitaldatacnt % 0xF);
delay_us(1);
//then using an array
if (input(diginDATA))
{
data[digitaldatacnt]=TRUE;
bit_set(txdig,digitaldatacnt);
}
else
data[digitaldatacnt]=FALSE;
digitaldatacnt = (++digitaldatacnt) & 0xF;
}
|
This does exactly the same as all 16 of your cases, except in needing the dataxx values stored as an array.
So:
| Code: |
static int1 data[16];
//and txdig as an int16
static int16 txdig;
|
Then what is now txdig0, would be the bottom byte of txdig, and txdig1 the high byte (look at make8).
Declaring these as static, ensures they are initialised t0 zero. |
|
|
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
| Posted: Sun Sep 04, 2016 8:35 pm |
|
|
Thank you Ttelmah for the answers an the time spent on this topic
Finally I am learning C and not writing in assembler 
With your DG() ROM used: 676 bytes (1%)
With my DG() ROM used: 1178 bytes (2%)
So the efficiency is clear!
I will try to implement what I have learned in other parts of my programs!
This is the program with your DG():
| Code: | ///////////////////////////////////////////////////////////////////
//test program for reading 16 digital inputs///////////////////////
///////////////////////////////////////////////////////////////////
#include <18F26K22.h>
#device ADC=10
#FUSES NOWDT //No Watch Dog Timer
#FUSES INTRC_IO //
#FUSES PLLEN
#FUSES NOFCMEN //Fail-safe clock monitor disabled
#FUSES NOIESO //Internal External Switch Over mode disabled
#FUSES PUT //Power Up Timer
#FUSES BROWNOUT
#FUSES BORV29 //Brownout reset at 2.85V
#FUSES NOPBADEN //PORTB pins are configured as digital I/O on RESET
#FUSES NOHFOFST //High Frequency INTRC waits until stable before clocking CPU
#FUSES NOLVP //No low voltage prgming, B3(PIC16) or B5(PIC18) used for I/O
#FUSES NOXINST //Extended set extension and Indexed Addressing mode disabled (Legacy mode)
#use delay(internal=64MHz)
#use rs232(baud=9600, UART1, stream=PORT1, errors)
#ZERO_RAM
#define diginsltA PIN_B0
#define diginsltB PIN_B1
#define diginsltC PIN_B2
#define diginsltOE PIN_B3//active L for data0; H for data1 via inverter
#define diginDATA PIN_B4
#define flashled PIN_C3
#define txflashled PIN_A4
int flashledcnt=0;
int digitaldatacnt=0;
#BYTE LATB=getenv("SFR:LATB")
static int1 data[16];
static int16 txdig;
int txdig0=0;
int txdig1=0;
int txwords=0;
int txwch=0;
int txtimecnat=0;
//#include "T22DG.h"
///////////////////////////////////////////////////////////////////
#INT_TIMER3
void TIMER3_isr(void)
{
flashledcnt++;
txtimecnat++;
if(flashledcnt==16)
{
output_toggle(flashled);//every 524ms
flashledcnt=0;
}
if(txtimecnat==64)
{
txdig0=make8(txdig,0);
txdig1=make8(txdig,1);
enable_interrupts(INT_TBE);//every 2.096 seconds
output_toggle(txflashled);
txtimecnat=0;
}
}
///////////////////////////////////////////////////////////////////
void DG(void)
{
LATB= (LATB & 0xF0) | (digitaldatacnt % 0xF);
delay_us(1);
//then using an array
if (input(diginDATA))
{
data[digitaldatacnt]=TRUE;
bit_set(txdig,digitaldatacnt);
}
else
{
data[digitaldatacnt]=FALSE;
}
digitaldatacnt = (++digitaldatacnt) & 0xF;
}
///////////////////////////////////////////////////////////////////
#INT_TBE
void TBE_isr(void)
{
switch(txwords)
{
case 0:
{
fputc(85,PORT1);
txwords++;
}
break;
case 1:
{
fputc(170,PORT1);
txwch=255;
txwords++;
}
break;
case 2:
{
fputc(txdig0,PORT1);
txwch=txwch+txdig0;
txwords++;
}
break;
case 3:
{
fputc(txdig1,PORT1);
txwch=txwch+txdig1;
txwords++;
}
break;
case 4:
{
fputc(txwch,PORT1);
txwords=0;
disable_interrupts(INT_TBE);
}
break;
}
}
///////////////////////////////////////////////////////////////////
void main()
{
port_b_pullups(0xFF);
setup_timer_3(T3_INTERNAL | T3_DIV_BY_8);//32.75 ms overflow
enable_interrupts(INT_TIMER3);
disable_interrupts(INT_RDA);
disable_interrupts(INT_TBE);
enable_interrupts(GLOBAL);
while(TRUE)
{
delay_us(10);
DG();
}
}
|
Everything work except that in the serial com I am getting:
| Quote: | | 55, AA, FF, FF, FD |
Even I am changing some inputs to '0' by jumpers on the board
I tried again my software with some changes, for example:
| Code: | if(input(diginDATA))
{
bit_set(txdig,12);
}
else
{
bit_clear(txdig,12);
} |
The previous version with adding bit values (like 1000,000 adding 128 to the byte) was giving garbage.
With the new version the serial com data is OK.
I suppose I am reading/sending not correct the data with your DG()
Can you tell me what is wrong?
Best wishes
Joe
Last edited by gjs_rsdi on Sun Sep 04, 2016 9:28 pm; edited 1 time in total |
|
|
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
| Posted: Sun Sep 04, 2016 9:28 pm |
|
|
Tried also with the added line in the code bellow, same FF, FF in the serial com
| Code: | void DG(void)
{
LATB= (LATB & 0xF0) | (digitaldatacnt % 0xF);
delay_us(1);
//then using an array
if (input(diginDATA))
{
data[digitaldatacnt]=TRUE;
bit_set(txdig,digitaldatacnt);
}
else
{
data[digitaldatacnt]=FALSE;
bit_clear(txdig,digitaldatacnt);//added line, joe
}
digitaldatacnt = (++digitaldatacnt) & 0xF;
} |
Best wishes
Joe |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 20059
|
|
| Posted: Mon Sep 05, 2016 12:57 am |
|
|
I don't see anywhere where you ever 'clear' txdig. Didn't see it in the original code, and same applies now. So once a bit is set, it'll never clear.
I see you have now added this.
Comments though why fiddle separating txdig?. Just use the bytes directly from it.
Make8, directly accesses the byte from the word. You can use it in your TX routine just as you were using the individual bytes. No point in wasting time (and space) copying the bytes.
Add a debug routine. If you haven't got a debugger, use a serial output, or pulses on a pin. My guess would be that possibly the delay for the key reading to settle needs to be a little longer, and in fact the code is seeing keys that aren't there.... |
|
|
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
| Posted: Tue Sep 06, 2016 5:06 pm |
|
|
Sorry for my delay with the answer, I made a lot of testings.
Corrected the TX routine per your advice Ttelmah.
While testing with a terminal getting the following results:
With any input to ground except ground input for digitaldatacnt 0000,1111 (the last input).
| Quote: | | 55, AA, FF, FF, FD |
If ground input for digitaldatacnt 0000,1111
| Quote: | | 55, AA, 00, 00, FF |
Testing in MPLAB simulator, digitaldatacnt increments from 0 to 15 and back, but LATB remain 0.
While testing with my modified DG(), I am getting the results correct on the terminal but the code is double than your code Ttelmah.
I tried to think using:
| Code: | | output_B(digitaldatacnt); |
but can't do it as pin_B4 is input, using B5 as output, using B6 as TX2 and using B7 as RX2
Was thinking to add
| Code: | #use fixed_io (port_outputs=pin, pin)
|
In the CCS help is written that
| Quote: | | make an I/O pin either input or output every time it is used |
but is port_outputs so how I can make port input?
Posting again the code, maybe can see something I don't see
| Code: | ///////////////////////////////////////////////////////////////////
//test program for reading 16 digital inputs///////////////////////
///////////////////////////////////////////////////////////////////
#include <18F26K22.h>
#device ADC=10
#FUSES NOWDT //No Watch Dog Timer
#FUSES INTRC_IO //
#FUSES PLLEN
#FUSES NOFCMEN //Fail-safe clock monitor disabled
#FUSES NOIESO //Internal External Switch Over mode disabled
#FUSES PUT //Power Up Timer
#FUSES BROWNOUT
#FUSES BORV29 //Brownout reset at 2.85V
#FUSES NOPBADEN //PORTB pins are configured as digital I/O on RESET
#FUSES NOHFOFST //High Frequency INTRC waits until stable before clocking CPU
#FUSES NOLVP //No low voltage prgming, B3(PIC16) or B5(PIC18) used for I/O
#FUSES NOXINST //Extended set extension and Indexed Addressing mode disabled (Legacy mode)
#use delay(internal=64MHz)
#use rs232(baud=9600, UART1, stream=PORT1, errors)
#ZERO_RAM
#define diginsltA PIN_B0
#define diginsltB PIN_B1
#define diginsltC PIN_B2
#define diginsltOE PIN_B3//active L for data0; H for data1 via inverter
#define diginDATA PIN_B4
#define flashled PIN_C3
#define txflashled PIN_A4
int flashledcnt=0;
int digitaldatacnt=0;
#BYTE LATB=getenv("SFR:LATB")
static int1 data[16];
static int16 txdig;
int txwords=0;
int txwch=0;
int txtimecnat=0;
//#include "T22DG.h"
///////////////////////////////////////////////////////////////////
#INT_TIMER3
void TIMER3_isr(void)
{
flashledcnt++;
txtimecnat++;
if(flashledcnt==16)
{
output_toggle(flashled);//every 524ms
flashledcnt=0;
}
if(txtimecnat==64)
{
enable_interrupts(INT_TBE);//every 2.096 seconds
output_toggle(txflashled);
txtimecnat=0;
}
}
///////////////////////////////////////////////////////////////////
void DG(void)
{
LATB= (LATB & 0xF0) | (digitaldatacnt % 0xF);
delay_us(10);
if (input(diginDATA))
{
data[digitaldatacnt]=TRUE;
bit_set(txdig,digitaldatacnt);
}
else
{
data[digitaldatacnt]=FALSE;
bit_clear(txdig,digitaldatacnt);
}
digitaldatacnt = (++digitaldatacnt) & 0xF;
}
///////////////////////////////////////////////////////////////////
#INT_TBE
void TBE_isr(void)
{
switch(txwords)
{
case 0:
{
fputc(85,PORT1);
txwords++;
}
break;
case 1:
{
fputc(170,PORT1);
txwch=255;
txwords++;
}
break;
case 2:
{
fputc(make8(txdig,0),PORT1);
txwch=txwch+make8(txdig,0);
txwords++;
}
break;
case 3:
{
fputc(make8(txdig,1),PORT1);
txwch=txwch+make8(txdig,1);
txwords++;
}
break;
case 4:
{
fputc(txwch,PORT1);
txwords=0;
disable_interrupts(INT_TBE);
}
break;
}
}
///////////////////////////////////////////////////////////////////
void main()
{
port_b_pullups(0xFF);
setup_timer_3(T3_INTERNAL | T3_DIV_BY_8);//32.75 ms overflow
enable_interrupts(INT_TIMER3);
disable_interrupts(INT_RDA);
disable_interrupts(INT_TBE);
enable_interrupts(GLOBAL);
while(TRUE)
{
delay_us(10);
DG();
}
}
|
Tried also with delay_us(100); same result
Best wishes
Joe |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 20059
|
|
| Posted: Wed Sep 07, 2016 1:31 am |
|
|
| Quote: |
but is port_outputs so how I can make port input?
|
The ones not specified as outputs, are inputs.....
This is based upon the chip's defaults. Basically the TRIS wakes set as 0b11111111. A '1' says the bit is an input. The command allows you to set bits to zero (output).
Understand this is why I accessed LATB directly. This does not change the TRIS. Assuming you have already set the TRIS how you want it or have already performed I/O on the pins, you can write to the LAT register and just update the output bits without changing anything else. |
|
|
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
| Posted: Fri Sep 09, 2016 6:24 pm |
|
|
Latest updated
The problem remain that using:
| Code: | void DG(void)
{
LATB= (LATB & 0xF0) | (digitaldatacnt % 0xF);
delay_us(10);
if (input(diginDATA))
{
data[digitaldatacnt]=TRUE;
bit_set(txdig,digitaldatacnt);
}
else
{
data[digitaldatacnt]=FALSE;
bit_clear(txdig,digitaldatacnt);
}
digitaldatacnt = (++digitaldatacnt) & 0xF;
} |
the output on the terminal will be always:
| Quote: | | 55, AA, FF, FF, FD |
With the long DG(), double the memory used, but the output is correct.
Tested in the MPLAB simulator both DG() acting the same
made:
| Code: | //static int16 txdig;
long txdig=0xFFFF; |
In both DG(); it becomes all 0 bit by bit.
One more thing regarding the simulator, while trying to see "data" from the Watch, beside appears: Unsupported Struct.
Tried many things/changes to solve the problem, nothing worked
Best wishes
Joe |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9632
Location: Greensville,Ontario
|
|
| Posted: Sat Sep 10, 2016 4:57 am |
|
|
Hmmm..
I wonder if you could make the code a lot simpler if you code as 2 groups of 8 switches instead of 1 of 16? The decoder is a 1 of 8 device anyway. Sending the data to PC is just 2 bytes, group 0-7 and group 8-15.
I know the code might not be at small or efficient, but overall it may be 'cleaner', easier to understand ?
Also when dealing with data like switches I use 'char' not 'int8'. Char is unsigned 8 bit, so 00-255 so there's no negative values I (or the compiler) have to deal with. It's an 'old school' thing from having to build translation tables and staring at ASCII code charts for decades...
Jay |
|
|
gjs_rsdi
Joined: 06 Feb 2006
Posts: 476
Location: Bali
|
|
| Posted: Sat Sep 10, 2016 5:58 am |
|
|
Hi Jay
I done it as you said in the beginning, two bytes, one for 0 to 7, one for 8 to 15 then switched to 0 to 15.
It makes no difference in the software (the longer DG()).
As I wrote, the long version DG() works OK, the shorter version give just FF, FF.
int is also unsigned 8 bit 0 to 255 and have no negative value.
I was just wondering what is missing in the short DG() that I am reading just FF, FF.
Best wishes
Joe |
|
|
|
|
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
