Luotao Fu wrote: > Hi Wolfgang, > > On Sat, Nov 28, 2009 at 09:17:57PM +0100, Wolfgang Grandegger wrote: >> Hi Fu, >> >> Luotao Fu wrote: >>> Hi Wolfgang, >>> >>> On Fri, Nov 27, 2009 at 11:14:57AM +0100, Wolfgang Grandegger wrote: >>> ... >>>> I don't want to invite people playing with the sample-point. To be >>>> clear, sample_point==0 will not be ignored. It means using CIA >>>> recommended sample points for the specified bit-rate. >>>> >>> The function is now splitte up in a simple set_bitrate and a expert >>> set_bitrate_samplepoint as you suggested. I Also changed name prefix >>> to can_ in API and renamed the library to libsocketcan. Hence the lib >>> is now available at: >>> git://git.pengutronix.de/git/tools/libsocketcan.git >>> The old URL still exists as a link for convenience though. >>> >>> I pinned the lib version to 0.0.4 and the updated canutils to 4.0.2. >>> There're tarballs for ready to use source on our ftp server. >> I finally had a closer look. I have attached two patches showing my >> enhancements and fixes. > > Thanks for the patches, comment see below. > >> Futhermore, I would change the name "mode" to "ctrlmode". "mode" is >> reserved for CAN operation modes like start, stop, or sleep. > > agreed. > >> It would also be nice to allow: >> >> $ canconfig can0 bitrate 125000 restart-ms 100 start >> > > This'd be indeed nice to have. Though we might have to change the parameter > parsing quite a littel. Let's see...
Yes, as-is, an option without args means "show" instead of "set", which makes it a bit more tricky. >> Add can_get_bittiming_const() >> >> Signed-off-by: Wolfgang Grandegger <[email protected]> > > Acked-by: Luotao Fu <[email protected]> > > applied, thx. > > <snip> > >> Add can_get_bittiming_const() and further fixes and improvements >> >> - s/can_modes/can_states/ >> - Use proper names for CAN error states >> - Use "-" as name seperator for input and output consequently >> - Add some more useful output to cmd_show_interface() >> >> Signed-off-by: Wolfgang Grandegger <[email protected]> >> > > minor nitpick see below. otherwise > Acked-by: Luotao Fu <[email protected]> > >> diff --git a/src/canconfig.c b/src/canconfig.c >> index d8b9167..0139a1c 100644 >> --- a/src/canconfig.c >> +++ b/src/canconfig.c >> @@ -37,11 +37,11 @@ >> #define MIN(a, b) ((a) < (b) ? (a) : (b)) >> #endif >> >> -const char *can_modes[CAN_STATE_MAX] = { >> - "ACTIVE", >> - "WARNING", >> - "PASSIVE", >> - "BUS OFF", >> +const char *can_states[CAN_STATE_MAX] = { >> + "ERROR-ACTIVE", >> + "ERROR-WARNING", >> + "ERROR-PASSIVE", >> + "BUS-OFF", >> "STOPPED", >> "SLEEPING" >> }; >> @@ -59,23 +59,24 @@ const char *can_modes[CAN_STATE_MAX] = { >> static void help(void) >> { >> fprintf(stderr, "usage:\n\t" >> - "canconfig <dev> bitrate { BR } [sample_point { SP }]\n\t\t" >> + "canconfig <dev> bitrate { BR } [sample-point { SP }]\n\t\t" >> "BR := <bitrate in Hz>\n\t\t" >> - "SP := <sample point {0...0.999}> (optional)\n\t" >> + "SP := <sample-point {0...0.999}> (optional)\n\t" >> "canconfig <dev> bittiming [ VALs ]\n\t\t" >> "VALs := <tq | prop-seg | phase-seg1 | phase-seg2 | sjw>\n\t\t" >> "tq <time quantum in ns>\n\t\t" >> - "prop_seg <no. in tq>\n\t\t" >> - "phase_seg1 <no. in tq>\n\t\t" >> - "phase_seg2 <no. in tq\n\t\t" >> + "prop-seg <no. in tq>\n\t\t" >> + "phase-seg1 <no. in tq>\n\t\t" >> + "phase-seg2 <no. in tq\n\t\t" >> "sjw <no. in tq> (optional)\n\t" >> - "canconfig <dev> restart-ms { RESTART_MS }\n\t\t" >> - "RESTART_MS := <autorestart interval in ms>\n\t" >> + "canconfig <dev> restart-ms { RESTART-MS }\n\t\t" >> + "RESTART-MS := <autorestart interval in ms>\n\t" >> "canconfig <dev> mode { MODE }\n\t\t" >> "MODE := <[loopback | listen-only | triple-sampling] >> [on|off]>\n\t" >> "canconfig <dev> {ACTION}\n\t\t" >> - "ACTION := <[start|stop|restart]>" >> - "canconfig <dev> clockfreq\n" >> + "ACTION := <[start|stop|restart]>\n\t" >> + "canconfig <dev> clockfreq\n\t" >> + "canconfig <dev> bittiming-constants\n" >> ); >> >> exit(EXIT_FAILURE); >> @@ -90,7 +91,7 @@ static void do_show_bitrate(int argc, char* argv[]) >> exit(EXIT_FAILURE); >> } else >> fprintf(stdout, >> - "%s bitrate: %u, sample point: %0.3f\n", >> + "%s bitrate: %u, sample-point: %0.3f\n", >> argv[1], bt.bitrate, >> (float)((float)bt.sample_point / 1000)); >> } >> @@ -202,6 +203,24 @@ static void do_show_bittiming(int argc, char *argv[]) >> bt.sjw, bt.brp); >> } >> >> +static void do_show_bittiming_const(int argc, char *argv[]) >> +{ >> + const char *name = argv[1]; >> + struct can_bittiming_const btc; >> + >> + if (can_get_bittiming_const(name, &btc) < 0) { >> + fprintf(stderr, "%s: failed to get bittiming_const\n", argv[1]); >> + exit(EXIT_FAILURE); >> + } else >> + fprintf(stdout, "%s bittiming-constants: name %s,\n\t" >> + "tseg1_min: %u, tseg1_max: %u, " >> + "tseg2_min: %u, tseg2_max: %u,\n\t" >> + "sjw_max %u, brp_min: %u, brp_max: %u, brp_inc: %u,\n", >> + name, btc.name, btc.tseg1_min, btc.tseg1_max, >> + btc.tseg2_min, btc.tseg2_max, btc.sjw_max, >> + btc.brp_min, btc.brp_max, btc.brp_inc); >> +} >> + > > to be consequent, we might also want "-"s instead of "_"s in the print > too. If you agree I'll change this here directly to avoid a resend. Yes, of course. Me not being consequent applying my suggested rules :-(. >> static void cmd_bittiming(int argc, char *argv[]) >> { >> if (argc >= 4) { >> @@ -223,7 +242,7 @@ static void do_show_state(int argc, char *argv[]) >> } >> >> if (state >= 0 && state < CAN_STATE_MAX) >> - fprintf(stdout, "%s state: %s\n", argv[1], can_modes[state]); >> + fprintf(stdout, "%s state: %s\n", argv[1], can_states[state]); >> else >> fprintf(stderr, "%s: unknown state\n", argv[1]); >> } >> @@ -257,6 +276,13 @@ static void cmd_clockfreq(int argc, char *argv[]) >> exit(EXIT_SUCCESS); >> } >> >> +static void cmd_bittiming_const(int argc, char *argv[]) >> +{ >> + do_show_bittiming_const(argc, argv); >> + >> + exit(EXIT_SUCCESS); >> +} >> + >> static void do_restart(int argc, char *argv[]) >> { >> if (can_do_restart(argv[1]) < 0) { >> @@ -395,7 +421,7 @@ static void do_show_restart_ms(int argc, char* argv[]) >> exit(EXIT_FAILURE); >> } else >> fprintf(stdout, >> - "%s restart_ms: %u\n", argv[1], restart_ms); >> + "%s restart-ms: %u\n", argv[1], restart_ms); >> } >> >> static void do_set_restart_ms(int argc, char* argv[]) >> @@ -430,10 +456,12 @@ static void cmd_baudrate(int argc, char *argv[]) >> static void cmd_show_interface(int argc, char *argv[]) >> { >> do_show_bitrate(argc, argv); >> + do_show_bittiming(argc, argv); >> do_show_state(argc, argv); >> do_show_restart_ms(argc, argv); >> - do_show_clockfreq(argc, argv); >> do_show_ctrlmode(argc, argv); >> + do_show_clockfreq(argc, argv); >> + do_show_bittiming_const(argc, argv); >> > > I kept the bittiming information out of show_interface to keep the > common output simple as possible. But you are so far right that these > are also essential informations. I might later add a verbose mode for > all these informations. In the common mode only the most important > infos like bitrate/state will be displayed than. I verbose mode would be useful, indeed. Wolfgang. _______________________________________________ Socketcan-users mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-users
