jrstepgui errors, default configurations, usability, code complexity
22 views
Skip to first unread message
iklln6
Feb 12, 2011, 11:51:12 PM2/12/11
to rstep
turn off all the popups. when i get a fragmented message back, i get
"rStep returned a badly formatted message" and everything goes grey
and i have to restart jrstepgui. when the 644 starts up and sends
'start' i get "rStep returned a badly formatted message" and
everything goes grey and i have to restart jrstepgui. when the 644
takes a little longer to start up everything stays grey and i have to
restart jrstepgui. i have M201 return MCP statuses as well, but i
need to change it i suppose because i get 10 popup messages about how
wrong that was. i just started jrstepgui to find more things for this
list, changed focus to safari, changed back to jrstepgui and
everything was grey. i had to force quit it because it wouldn't shut
itself down. I hit the [x] button to manually step a direction, it
works, i keep clicking it and i get a OK that's fragmented [firmware
problem] and jrstepgui pops up "rStep returned a badly formatted
message". everything is grey and i have to restart jrstepgui.
so let me expand
-turn off the popup
-whatever it is that disables all the buttons, delete that suicide
code
stop overcomplicating the damn configurations. a simple
"if(CHECKSUM_ERROR) {load_default_configurations();
Serial.println("defaults loaded"); } " then in jrstepgui for gods
sake have it catch the "defaults loaded" message and display a message
in the messages tab for information, NOT a popup. when i flash a chip
that doesn't have configurations on it, i see the headache message, go
to configure. in my personal situation, i spend a minute clicking
through the popupboxes of a scared jrstepgui that got more information
than it wanted, then i make a change and click "OK". I get another
popup that says ""rStep returned a badly formatted message" because
the OK was fragmented, and all the buttons go grey and i have to
restart jrstepgui. the only reason i have configurations to begin
with is because i wrote my own function to load defaults
configurations. when i restart, they stay. when jrstepgui does it,
they go away. configurations being null MUST be handled silently and
efficiently if you want this to be open source. nobody wants to deal
with this headache every time they upload a revised version of the
firmware to the board.
----in the parser:
(M)
case 252:
config_restore_defaults();
return_status = RET_OK;
break;
----in config tab of the firmware:
/*nash--11 DEC*/
void config_restore_defaults(void) {
uint16_t i;
uint8_t checksum = 0;
config_t *cfg = &config;
cfg->steps_inch.x = 4064.0;
cfg->steps_inch.y = 4064.0;
cfg->steps_inch.z = 4064.0;
cfg->current.x = 180;
cfg->current.y = 180;
cfg->current.z = 180;
cfg->max_feedrate.x = 15;
cfg->max_feedrate.y = 15;
cfg->max_feedrate.z = 13;
cfg->stepping = 1;
cfg->dir = 1;
cfg->motorSpeed = 128;
uint8_t *ptr = (uint8_t*)(&config);
for (i=0; i<sizeof(struct config_t); i++) {
EEPROM.write(i,ptr[i]);
checksum += ptr[i];
}
EEPROM.write(i+1,checksum);
config_read();
}
/* /nash--11 DEC*/
this is what the checksum portion of config_read() should be:
if (EEPROM.read(i+1) != checksum) {
//XXX add more error checking code here [nash] NO NO NO more error
checking code here
// Serial.println("ERR CHECKSUM"); //[nash] NO
Serial.print(0x44); //OR just push the byte out. This will
be easier to read from jrstepgui an easier to handle (termination char
might be necessary)
}
in jrstepgui
if(read == 0x44){
if(verbose debugging is on) bombard user with error popups
else {
display message in message box
printf("M252\r");
}
}
********on code complexity:
void mcp4351_init(void) {
uint8_t retry = 3;
//configure pins
pinMode(MSP4351_CS, OUTPUT);
pinMode(SCK, OUTPUT);
pinMode(MOSI, OUTPUT);
pinMode(SS, OUTPUT); //just to be safe
pinMode(MISO, INPUT);
digitalWrite(SCK, LOW);
digitalWrite(MOSI, LOW);
digitalWrite(MISO, LOW);
//restore values
SPI_ON();
while (retry--) {
if (mcp4351_setWiper(CHAN_X, (uint16_t)(config.current.x<<1)) &&
mcp4351_setWiper(CHAN_Y, (uint16_t)(config.current.y<<1)) &&
mcp4351_setWiper(CHAN_Z, (uint16_t)(config.current.z<<1))) {
return;
}
}
Serial.println("ERR INIT");
while(1); //lock up device as we can't set the currents
}
vs.
#define _CURR(a) map(a,0,CURRENT_MAX,0,255)
void mcp4351_init(void) {
MCP.init();
MCP.setwiper(CHAN_X,_CURR(config.current.x));
MCP.setwiper(CHAN_Y,_CURR(config.current.y));
MCP.setwiper(CHAN_Z,_CURR(config.current.z));
}
and things like this:
//returns zero if value does not exist.
double getValue(const char x) {
int i;
//find entry
for (i=0; i<commandLength; i++) {
if (command_list[i].type == x) break;
}
//did we find or run out?
if (i==commandLength) return 0;
return command_list[i].value;
}
vs
// change: RX_Length = commandLength
//change: RX_Commands[] = command_list[]
double getValue(const char x) {
for (int i = 0; i <= RX_Length; i++) if (RX_Commands[i].type ==
x) return RX_Commands[i].value;
return 0;
}
code readability(!) unnecessary lines(!)
imagine you're looking at the code having never seen it before, you're
already pissed off because you have to jump from tab to tab looking up
definitions to pointers to structures of arrays of pointers to
configuration values, and you see command_exists and command_list.
gerber commands? positioning commands? firmware commands? i know you
know, but it's ambiguous to people that didn't write it. and when
looking at the function as a whole, you see a whole bunch of
unnecessary stuff that only adds to the amount of information one
needs to process while looking for target(real) information.
and stuff like this:
#define ENABLE 2 //low enabled
#define MS1 17
#define MS2 16
#define RST 3
#define SLP 3
#define STEP_X 14
#define STEP_Y 12
etc
if you're going to have so many structures, pointers, arrays, etc then
for the love of god have some better naming convention to make the
real values easier to identify. like below in a board.h file:
/*STEPPER STEP REGISTERS*/
#define STEP_DDR DDRD
#define STEP_PORT PORTD
#define STEP_PIN PIND
#define dX 14 /*PD6*/
#define pX 6 //PD
#define dY 12 /*PD4*/
#define pY 4 //PD
#define dZ 10 /*PD2*/
#define pZ 2 //PD
and hell throw in a simpler step setting definition like this:
#define _SET_STEP(a) (a==1)?
((MS_PORT&=~(1<<pMS1);MS_PORT&=~(1<<pMS2))):((a==2)?((MS_PORT|
=(1<<pMS1);MS_PORT&=~(1<<pMS2))):((a==4)?((MS_PORT&=~(1<<pMS1);MS_PORT|
=(1<<pMS2))):((a==16)?((MS_PORT|=(1<<pMS1);MS_PORT|=(1<<pMS2))):
((MS_PORT&=~(1<<pMS1);MS_PORT&=~(1<<pMS2))))))
which may be a mess to look at, the commands are easy to follow as
just turning pins on and off (you only have to make a pointer look-up
for the ports and bits, which are set side by side when formatted like
that above). so you set your stepsize as:
_SET_STEPSIZE(axis->stepsize); //given stepsize is defined as
1,2,4,or 16
ok ok one more:
#define LED1_ON() digitalWrite(LED1,LOW);
#define LED1_OFF() digitalWrite(LED1,HIGH);
#define LED2_ON() digitalWrite(LED2,LOW);
#define LED2_OFF() digitalWrite(LED2,HIGH);
that is nice and simple, but why not just use:
#define LED1(x) DDRA |= (1<<4); ((x)==(1)?(LED_PORT &= ~(1<<pLED1)):
(LED_PORT |= (1<<pLED1))); /*active low*/
#define LED2(x) DDRA |= (1<<1); ((x)==(1)?(LED_PORT &= ~(1<<pLED2)):
(LED_PORT |= (1<<pLED2))); /*active low*/
and in the functions LED1(1) or LED1(0). two 1-D functions opposed to
four 1-D functions is simpler to follow.
i swear this is the last:
this is what i use for stepping
/*Single step functions, ensures outputs; A3984 requires ~9us ON time
to trigger a step, x sets the delay between steps + 25us */
#define sX(x,t) STEP_DDR |= (1<<pX); (STEP_PORT |= (1<<pX));
delayMicroseconds(t); (STEP_PORT &= ~(1<<pX)); delay(x);
#define sY(x,t) STEP_DDR |= (1<<pY); (STEP_PORT |= (1<<pY));
delayMicroseconds(t); (STEP_PORT &= ~(1<<pY)); delay(x);
#define sZ(x,t) STEP_DDR |= (1<<pZ); (STEP_PORT |= (1<<pZ));
delayMicroseconds(t); (STEP_PORT &= ~(1<<pZ)); delay(x);
it ensures the pins are outputs, turns the motor on and takes a step,
then turns the pin off. p.s. wtf is this
_STEP_PORT &= ~a->direct_step_pin; ...direct_step_pin? it's almost
as poorly named as minMax_pin (LIMIT) and config_t (t stands
for ...) . i had to jump back to the definitions again to search for
what this is. but +1 jump is meaningless when added to 1000, except
that 1001 is where people will draw the line and say fuck it when all
the code real code on any given page is a mixture of <>~/=* and the
rest is nothing but pointers. those pointers NEED to be named well.
about every time i start working on making the cnc come to life i end
up running out of time (and my break point is ~8 hours at minimum).
by the time i can get up to work on it again, i repeat the process
updating the newest code after the fresh upload of the new code
fails. i've rewritten the code a few times now, and it always comes
down to a configuration of some sort being bad (or nonexistent), or a
pin being misdefined, or the MCP (fixed and consistent with the
library), and troubleshooting those kind of minor errors should NOT be
taking 8+ hours.
just out of curiosity, if the firmware isn't going to be compatible
with any other software or system other than jrstepgui, why not just
have jrstepgui do all the calculations and just make the firmware
responsible for stepping, limits, and setting the MCP? seems asinine
to make an 8-bit chip make all those calculations when it's only going
to making those calculations when being controlled by jrstepgui. and
if the intent is to make it universal, why not make it compatible with
Mach3 or EMC2?
"rStep returned a badly formatted message" and everything goes grey
and i have to restart jrstepgui. when the 644 starts up and sends
'start' i get "rStep returned a badly formatted message" and
everything goes grey and i have to restart jrstepgui. when the 644
takes a little longer to start up everything stays grey and i have to
restart jrstepgui. i have M201 return MCP statuses as well, but i
need to change it i suppose because i get 10 popup messages about how
wrong that was. i just started jrstepgui to find more things for this
list, changed focus to safari, changed back to jrstepgui and
everything was grey. i had to force quit it because it wouldn't shut
itself down. I hit the [x] button to manually step a direction, it
works, i keep clicking it and i get a OK that's fragmented [firmware
problem] and jrstepgui pops up "rStep returned a badly formatted
message". everything is grey and i have to restart jrstepgui.
so let me expand
-turn off the popup
-whatever it is that disables all the buttons, delete that suicide
code
stop overcomplicating the damn configurations. a simple
"if(CHECKSUM_ERROR) {load_default_configurations();
Serial.println("defaults loaded"); } " then in jrstepgui for gods
sake have it catch the "defaults loaded" message and display a message
in the messages tab for information, NOT a popup. when i flash a chip
that doesn't have configurations on it, i see the headache message, go
to configure. in my personal situation, i spend a minute clicking
through the popupboxes of a scared jrstepgui that got more information
than it wanted, then i make a change and click "OK". I get another
popup that says ""rStep returned a badly formatted message" because
the OK was fragmented, and all the buttons go grey and i have to
restart jrstepgui. the only reason i have configurations to begin
with is because i wrote my own function to load defaults
configurations. when i restart, they stay. when jrstepgui does it,
they go away. configurations being null MUST be handled silently and
efficiently if you want this to be open source. nobody wants to deal
with this headache every time they upload a revised version of the
firmware to the board.
----in the parser:
(M)
case 252:
config_restore_defaults();
return_status = RET_OK;
break;
----in config tab of the firmware:
/*nash--11 DEC*/
void config_restore_defaults(void) {
uint16_t i;
uint8_t checksum = 0;
config_t *cfg = &config;
cfg->steps_inch.x = 4064.0;
cfg->steps_inch.y = 4064.0;
cfg->steps_inch.z = 4064.0;
cfg->current.x = 180;
cfg->current.y = 180;
cfg->current.z = 180;
cfg->max_feedrate.x = 15;
cfg->max_feedrate.y = 15;
cfg->max_feedrate.z = 13;
cfg->stepping = 1;
cfg->dir = 1;
cfg->motorSpeed = 128;
uint8_t *ptr = (uint8_t*)(&config);
for (i=0; i<sizeof(struct config_t); i++) {
EEPROM.write(i,ptr[i]);
checksum += ptr[i];
}
EEPROM.write(i+1,checksum);
config_read();
}
/* /nash--11 DEC*/
this is what the checksum portion of config_read() should be:
if (EEPROM.read(i+1) != checksum) {
//XXX add more error checking code here [nash] NO NO NO more error
checking code here
// Serial.println("ERR CHECKSUM"); //[nash] NO
Serial.print(0x44); //OR just push the byte out. This will
be easier to read from jrstepgui an easier to handle (termination char
might be necessary)
}
in jrstepgui
if(read == 0x44){
if(verbose debugging is on) bombard user with error popups
else {
display message in message box
printf("M252\r");
}
}
********on code complexity:
void mcp4351_init(void) {
uint8_t retry = 3;
//configure pins
pinMode(MSP4351_CS, OUTPUT);
pinMode(SCK, OUTPUT);
pinMode(MOSI, OUTPUT);
pinMode(SS, OUTPUT); //just to be safe
pinMode(MISO, INPUT);
digitalWrite(SCK, LOW);
digitalWrite(MOSI, LOW);
digitalWrite(MISO, LOW);
//restore values
SPI_ON();
while (retry--) {
if (mcp4351_setWiper(CHAN_X, (uint16_t)(config.current.x<<1)) &&
mcp4351_setWiper(CHAN_Y, (uint16_t)(config.current.y<<1)) &&
mcp4351_setWiper(CHAN_Z, (uint16_t)(config.current.z<<1))) {
return;
}
}
Serial.println("ERR INIT");
while(1); //lock up device as we can't set the currents
}
vs.
#define _CURR(a) map(a,0,CURRENT_MAX,0,255)
void mcp4351_init(void) {
MCP.init();
MCP.setwiper(CHAN_X,_CURR(config.current.x));
MCP.setwiper(CHAN_Y,_CURR(config.current.y));
MCP.setwiper(CHAN_Z,_CURR(config.current.z));
}
and things like this:
//returns zero if value does not exist.
double getValue(const char x) {
int i;
//find entry
for (i=0; i<commandLength; i++) {
if (command_list[i].type == x) break;
}
//did we find or run out?
if (i==commandLength) return 0;
return command_list[i].value;
}
vs
// change: RX_Length = commandLength
//change: RX_Commands[] = command_list[]
double getValue(const char x) {
for (int i = 0; i <= RX_Length; i++) if (RX_Commands[i].type ==
x) return RX_Commands[i].value;
return 0;
}
code readability(!) unnecessary lines(!)
imagine you're looking at the code having never seen it before, you're
already pissed off because you have to jump from tab to tab looking up
definitions to pointers to structures of arrays of pointers to
configuration values, and you see command_exists and command_list.
gerber commands? positioning commands? firmware commands? i know you
know, but it's ambiguous to people that didn't write it. and when
looking at the function as a whole, you see a whole bunch of
unnecessary stuff that only adds to the amount of information one
needs to process while looking for target(real) information.
and stuff like this:
#define ENABLE 2 //low enabled
#define MS1 17
#define MS2 16
#define RST 3
#define SLP 3
#define STEP_X 14
#define STEP_Y 12
etc
if you're going to have so many structures, pointers, arrays, etc then
for the love of god have some better naming convention to make the
real values easier to identify. like below in a board.h file:
/*STEPPER STEP REGISTERS*/
#define STEP_DDR DDRD
#define STEP_PORT PORTD
#define STEP_PIN PIND
#define dX 14 /*PD6*/
#define pX 6 //PD
#define dY 12 /*PD4*/
#define pY 4 //PD
#define dZ 10 /*PD2*/
#define pZ 2 //PD
and hell throw in a simpler step setting definition like this:
#define _SET_STEP(a) (a==1)?
((MS_PORT&=~(1<<pMS1);MS_PORT&=~(1<<pMS2))):((a==2)?((MS_PORT|
=(1<<pMS1);MS_PORT&=~(1<<pMS2))):((a==4)?((MS_PORT&=~(1<<pMS1);MS_PORT|
=(1<<pMS2))):((a==16)?((MS_PORT|=(1<<pMS1);MS_PORT|=(1<<pMS2))):
((MS_PORT&=~(1<<pMS1);MS_PORT&=~(1<<pMS2))))))
which may be a mess to look at, the commands are easy to follow as
just turning pins on and off (you only have to make a pointer look-up
for the ports and bits, which are set side by side when formatted like
that above). so you set your stepsize as:
_SET_STEPSIZE(axis->stepsize); //given stepsize is defined as
1,2,4,or 16
ok ok one more:
#define LED1_ON() digitalWrite(LED1,LOW);
#define LED1_OFF() digitalWrite(LED1,HIGH);
#define LED2_ON() digitalWrite(LED2,LOW);
#define LED2_OFF() digitalWrite(LED2,HIGH);
that is nice and simple, but why not just use:
#define LED1(x) DDRA |= (1<<4); ((x)==(1)?(LED_PORT &= ~(1<<pLED1)):
(LED_PORT |= (1<<pLED1))); /*active low*/
#define LED2(x) DDRA |= (1<<1); ((x)==(1)?(LED_PORT &= ~(1<<pLED2)):
(LED_PORT |= (1<<pLED2))); /*active low*/
and in the functions LED1(1) or LED1(0). two 1-D functions opposed to
four 1-D functions is simpler to follow.
i swear this is the last:
this is what i use for stepping
/*Single step functions, ensures outputs; A3984 requires ~9us ON time
to trigger a step, x sets the delay between steps + 25us */
#define sX(x,t) STEP_DDR |= (1<<pX); (STEP_PORT |= (1<<pX));
delayMicroseconds(t); (STEP_PORT &= ~(1<<pX)); delay(x);
#define sY(x,t) STEP_DDR |= (1<<pY); (STEP_PORT |= (1<<pY));
delayMicroseconds(t); (STEP_PORT &= ~(1<<pY)); delay(x);
#define sZ(x,t) STEP_DDR |= (1<<pZ); (STEP_PORT |= (1<<pZ));
delayMicroseconds(t); (STEP_PORT &= ~(1<<pZ)); delay(x);
it ensures the pins are outputs, turns the motor on and takes a step,
then turns the pin off. p.s. wtf is this
_STEP_PORT &= ~a->direct_step_pin; ...direct_step_pin? it's almost
as poorly named as minMax_pin (LIMIT) and config_t (t stands
for ...) . i had to jump back to the definitions again to search for
what this is. but +1 jump is meaningless when added to 1000, except
that 1001 is where people will draw the line and say fuck it when all
the code real code on any given page is a mixture of <>~/=* and the
rest is nothing but pointers. those pointers NEED to be named well.
about every time i start working on making the cnc come to life i end
up running out of time (and my break point is ~8 hours at minimum).
by the time i can get up to work on it again, i repeat the process
updating the newest code after the fresh upload of the new code
fails. i've rewritten the code a few times now, and it always comes
down to a configuration of some sort being bad (or nonexistent), or a
pin being misdefined, or the MCP (fixed and consistent with the
library), and troubleshooting those kind of minor errors should NOT be
taking 8+ hours.
just out of curiosity, if the firmware isn't going to be compatible
with any other software or system other than jrstepgui, why not just
have jrstepgui do all the calculations and just make the firmware
responsible for stepping, limits, and setting the MCP? seems asinine
to make an 8-bit chip make all those calculations when it's only going
to making those calculations when being controlled by jrstepgui. and
if the intent is to make it universal, why not make it compatible with
Mach3 or EMC2?
NULL
Feb 20, 2011, 7:45:39 PM2/20/11
to rstep
what does ERR addObj FULL mean?
Reza Naima
Feb 20, 2011, 10:18:11 PM2/20/11
to rs...@googlegroups.com
I think it means you have too many options in one command, though I
need to go to the source to check. What caused it? It's also
configurable so if you need longer arguments, you can adjust that
parameter.
reza
reza
NULL
Sunday, February 20, 2011 4:45 PM
what does ERR addObj FULL mean?
iklln6
Saturday, February 12, 2011 8:51 PM
NULL
Feb 21, 2011, 7:18:30 PM2/21/11
to rstep
here is the code. it is for drawing a circle
G2 X 39.518 Y 46.268 I 16.018 J 16.018
G2 X 7.482 Y 14.232 I -16.018 J -16.018
G2 X 39.518 Y 46.268 I 16.018 J 16.018
G2 X 7.482 Y 14.232 I -16.018 J -16.018
G0 X 8.246 Y 61.246
G2 X 38.254 Y 91.254 I 15.004 J 15.004
G2 X 8.246 Y 61.246 I -15.004 J -15.004
G2 X 38.254 Y 91.254 I 15.004 J 15.004
G2 X 8.246 Y 61.246 I -15.004 J -15.004
On Feb 20, 10:18 pm, Reza Naima <r...@reza.net> wrote:
> I think it means you have too many options in one command, though I need
> to go to the source to check. What caused it? It's also configurable
> so if you need longer arguments, you can adjust that parameter.
>
> reza
>
>
>
>
>
>
>
> > ------------------------------------------------------------------------
>
> > NULL <mailto:bigbodysmallbr...@gmail.com>
>
> > iklln6 <mailto:michaeldn...@gmail.com>
> ...
>
> read more »
G2 X 39.518 Y 46.268 I 16.018 J 16.018
G2 X 7.482 Y 14.232 I -16.018 J -16.018
G2 X 39.518 Y 46.268 I 16.018 J 16.018
G2 X 7.482 Y 14.232 I -16.018 J -16.018
G0 X 8.246 Y 61.246
G2 X 38.254 Y 91.254 I 15.004 J 15.004
G2 X 8.246 Y 61.246 I -15.004 J -15.004
G2 X 38.254 Y 91.254 I 15.004 J 15.004
G2 X 8.246 Y 61.246 I -15.004 J -15.004
On Feb 20, 10:18 pm, Reza Naima <r...@reza.net> wrote:
> I think it means you have too many options in one command, though I need
> to go to the source to check. What caused it? It's also configurable
> so if you need longer arguments, you can adjust that parameter.
>
> reza
>
>
>
>
>
>
>
>
> > NULL <mailto:bigbodysmallbr...@gmail.com>
> > Sunday, February 20, 2011 4:45 PM
>
> > what does ERR addObj FULL mean?
>
> > ------------------------------------------------------------------------
>
> > what does ERR addObj FULL mean?
>
>
> > iklln6 <mailto:michaeldn...@gmail.com>
>
> read more »
Reza Naima
Feb 21, 2011, 9:38:14 PM2/21/11
to rs...@googlegroups.com
The code does not support spaces between the the letter and value.
So you would need
G2 X39.518 Y46.268 I16.018 J16.018
Having spaces generates twice as many objects and is generating the error. Is that a standard format for gcode, as I've not seen it before. If so, I can look into support it.
Reza
G2 X39.518 Y46.268 I16.018 J16.018
Having spaces generates twice as many objects and is generating the error. Is that a standard format for gcode, as I've not seen it before. If so, I can look into support it.
Reza
NULL
Monday, February 21, 2011 4:18 PM
here is the code. it is for drawing a circle
G2 X 39.518 Y 46.268 I 16.018 J 16.018
G2 X 7.482 Y 14.232 I -16.018 J -16.018
G2 X 39.518 Y 46.268 I 16.018 J 16.018
G2 X 7.482 Y 14.232 I -16.018 J -16.018
G0 X 8.246 Y 61.246
G2 X 38.254 Y 91.254 I 15.004 J 15.004
G2 X 8.246 Y 61.246 I -15.004 J -15.004
G2 X 38.254 Y 91.254 I 15.004 J 15.004
G2 X 8.246 Y 61.246 I -15.004 J -15.004
Reza Naima
Sunday, February 20, 2011 7:18 PM
I think it means you have too many options in one command, though I need to go to the source to check. What caused it? It's also configurable so if you need longer arguments, you can adjust that parameter.
reza
Sunday, February 20, 2011 4:45 PM
what does ERR addObj FULL mean?
Reply all
Reply to author
Forward
0 new messages