uni-t-ut8802e: Initial commit #220
Conversation
| */ | ||
|
|
||
| static const int8_t range_volt[] = {-3, 0}; | ||
| static const int8_t range_amp[] = {-6, -3, 0}; |
There was a problem hiding this comment.
range_amp[2] is never used. The initializer list can be shortened.
There was a problem hiding this comment.
Range_amp[2], I used to choose a range for the current measurement in 168 to 194 lines.
There was a problem hiding this comment.
No, range_amp[2] is not used.
|
|
||
| if (clc_checksum > 256 && want_checksum >= 128) | ||
| want_checksum = want_checksum - 128; | ||
|
|
There was a problem hiding this comment.
The checksum is calculated over the first 8 bytes including the FRAME_MAGIC byte (0xAC). If the checksum is in the range 0xAC - 0xFF, i.e. the sum of the remaining bytes is less than 0x54, the checksum should be found in pkt[8] with bit 7 set. But if the checksum is 0x100 or more, bit 7 of the checksum will be reset.
This is a strange definition.
There was a problem hiding this comment.
Thank you very much. I made a mistake here. I fixed it now. Bit 7 is always cleared.
0xac 0x04 0x10 0x20 0x01 0x33 0x04 0x18 -> Sum of bytes 0x118,
0xac 0x06 0x00 0x00 0x00 0x31 0x04 0x67 (103) -> Sum of bytes 0xE7 (231)
0xac 0x04 0x88 0x99 0x01 0x33 0x04 0x09 -> Sum of bytes 0x209
0xac 0x04 0x89 0x69 0x01 0x33 0x04 0x5a (90)-> Sum of bytes 0x1DA (474)
There was a problem hiding this comment.
Sorry for the late review, but here it is.
A bunch of mostly small comments, easily fixable. A couple of general notes:
- Your code uses spaces for indentation, please use tabs instead.
- Have you checked whether this protocol might already be supported by some other driver? This may well be driven by a generic DMM chip, and we already support lots of those (see https://sigrok.org/wiki/Multimeter_ICs). The easiest way to check is just to open up the device and check markings on the main IC.
| case SR_CONF_DATA_SOURCE: | ||
| *data = g_variant_new_string(data_sources[0]); | ||
| break; | ||
| // TO DO ADD SR_CONF_MEASURED_QUANTITY and SR_CONF_RANGE |
There was a problem hiding this comment.
Please don't leave TODOs in the code that's going up to mainline. It's ok if your driver doesn't yet support all features the hardware has, but the driver should just be functional for the stuff it does support. You can make a note of features it still needs in the commit message or (much preferred) the wiki.
|
|
||
|
|
||
| static const char *channel_names[] = { | ||
| "Main", |
There was a problem hiding this comment.
Regular multimeters like this generally use P1 as the channel name (for probe 1).
|
|
||
| static const char *data_sources[] = { | ||
| "Live", | ||
| }; |
There was a problem hiding this comment.
Since this multimeter only has live measurements, i.e. no storage mode, there is no need to implement SR_CONF_DATA_SOURCE in this driver. So this can go, and the entry in devopts[], and the config_get()/config_list() entries as well.
| devices = g_slist_append(devices, sdi); | ||
|
|
||
| return std_scan_complete(di, devices); | ||
|
|
| * Frame | func | value | comma | settings | checksum | | ||
| * ac | 1b | 45 01 00 | 33 | 04 | 44 | | ||
| * - The frame to send a command to the multimeter is unknown. | ||
| */ |
There was a problem hiding this comment.
This really belongs in the wiki, not in the code. Can you please create a page for this hardware? @abraxa will hook you up with an account. Document as much detail of the protocol as you can.
| serial = sdi->conn; | ||
|
|
||
| if (devc->packet_len == sizeof(devc->packet)) { | ||
| (void)process_packet(sdi, &devc->packet[0], devc->packet_len); |
There was a problem hiding this comment.
Why not use the return code? Might be an error.
| return TRUE; | ||
|
|
||
| if (revents & G_IO_IN) | ||
| (void)ut8802e_receive_data(sdi); |
There was a problem hiding this comment.
If you don't want to know if that function returned an error, just rewrite the function to not return one.
|
|
||
| #define PACKET_SIZE 32 | ||
|
|
||
| #define FRAME_MAGIC 0xac /* AC */ |
There was a problem hiding this comment.
The comment seems unnecessary.
|
|
||
| enum ut8802e_channel_idx { | ||
| ut8802e_CH_MAIN, | ||
| }; |
There was a problem hiding this comment.
There is only one channel, this seems unnecessary.
| want_checksum = want_checksum - 128; | ||
|
|
||
| got_checksum = R8(&pkt[checksum_dlen]); | ||
| sr_spew("Checksum: %d, Got: %d, ALL %d", want_checksum, got_checksum, clc_checksum); |
There was a problem hiding this comment.
Once you know you've got the checksum right, no need to spew this debug output.
Implementation of the driver for Uni-t UT8802E bench multimeter. The protocol was reconstructed manually. I suppose that the multimeter supports only reading the measurement.