initial update for -j json #575
Conversation
I have added cjson to to the repo This is my first pass of adding -j, there is more work to do
There was a problem hiding this comment.
Thanks for your submission @benroeder!
Overall great job, but it needs some work to move on with this - please read comments below.
But main points are:
- Consider using encoder-only json C library that is as small as possible, e.g.
mkjson- it is 10x smaller than cJSON. - Try making emitted json as small as possible: avoid emitting empty or false values. E.g. always emitting
"indicator": falsein list of flags is hardly useful. Try using shorter field names (but still make them understandable) - e.g. consider usingnportsinstead ofnum_ports- this will make json smaller. - Retain backwards compatibility as much as possible - e.g. existing code is using
pppsfor per-port power switching mode supported - consider using it instead ofper-port. - For json generating functions, avoid copying large blocks from old non-json functions.
- Try to make output json as small as possible and don't emit info that is rarely used - basically anything that is not related to current port power state. If you want it to be still available, consider adding --verbose/-V flag that would make it really verbose.
| $(RM) $(PROGRAM).o $(PROGRAM).dSYM $(PROGRAM) | ||
| $(RM) $(OBJECTS) $(PROGRAM).dSYM $(PROGRAM) | ||
|
|
||
| .PHONY: all install clean |
| @@ -0,0 +1,300 @@ | |||
| /* | |||
| @@ -18,6 +18,10 @@ | |||
| #include <getopt.h> | |||
| #include <errno.h> | |||
| #include <ctype.h> | |||
| #include <stdbool.h> | |||
There was a problem hiding this comment.
You don't need stdbool.h - commenting this out still works - please remove.
| @@ -180,6 +184,38 @@ struct usb_port_status { | |||
| #define HUB_CHAR_TTTT 0x0060 /* TT Think Time mask */ | |||
| #define HUB_CHAR_PORTIND 0x0080 /* per-port indicators (LEDs) */ | |||
|
|
|||
| /* | |||
| * USB Speed definitions | |||
There was a problem hiding this comment.
Can you please link source where you got these speed definitions from? Perhaps USB spec or something?
| int is_mass_storage_device(struct libusb_device *dev) | ||
| { | ||
| struct libusb_config_descriptor *config; | ||
| int ret = 0; |
There was a problem hiding this comment.
Please use rc instead of ret to be consistent with other code
| const char* power_switching_mode; | ||
| switch (hub->lpsm) { | ||
| case HUB_CHAR_INDV_PORT_LPSM: | ||
| power_switching_mode = "per-port"; |
There was a problem hiding this comment.
This mode is already set to ppps in get_device_description, make it consistent?
| power_switching_mode = "ganged"; | ||
| break; | ||
| default: | ||
| power_switching_mode = "unknown"; |
There was a problem hiding this comment.
This is set to nops in get_device_description (no power switching).
| struct libusb_device_handle* devh = NULL; | ||
| int rc = libusb_open(hub->dev, &devh); | ||
| if (rc == 0) { | ||
| for (int port = 1; port <= hub->nports; port++) { |
There was a problem hiding this comment.
This code has big copy/paste chunk from get_device_description(). Is there a way to avoid this? Ideally make it into one function?
| int mask; | ||
| const char* name; | ||
| } flag_defs[] = { | ||
| {USB_PORT_STAT_CONNECTION, "connection"}, |
There was a problem hiding this comment.
For brevity, I would not emit any flag that has false value - it would considerably reduce output size
| } | ||
|
|
||
| // Helper function to determine port speed | ||
| void get_port_speed(int port_status, char** speed_str, int64_t* speed_bits) |
There was a problem hiding this comment.
speed_bits should be speed_bps (bits per second). That should be in emitted json as well.
I have added cjson to the repo
This is my first pass of adding -j, there is more work to do, but wanted your opinion first before I carry on