Update vc96.c #234
Update vc96.c #234
Conversation
Fixes wrong detection and calculation.
There was a problem hiding this comment.
In (new) line 73-77 curly brackets are added while this style is not found elsewhere for one line statements in if-clauses. The semicolon is unnecessary.
!strncmp(buf, " ", 3)
These calls in line 84 and 96 compare a string of 2 spaces and a terminating zero with the buffer. This can be done simpler with a strcmp(buf, " ")
Line 93-94 sets is_ohm if the buffer contains "OHM". It leaves is_ohm untouched if the buffer contains 2 spaces.
But line 95-96 set is_diode if the buffer contains "DIO". It also set is_diode if the buffer contains 2 spaces AND is_volt AND is_milli are set. This looks weird. At least both members is_ohm and is_volt will be evaluated later in handle_flags() where analog->meaning->mq and analog->meaning->unit will be overwritten.
Line 97-98 sets hfe only if the buffer contains "hfe" AND all members (is_ampere, is_volt, isresistance, is_diode) are not set. Is this really special for"hfe"?
Line 112-128 checks for units. Old order A, mA, µA,V,mV. New order mA,µA,A,mV,V. The order for check for K and M has also changed. Is this really necessary and is it better?
Line 148-150 contains unnecessary curly brackets and semicolon.
Line 159 looks like the real fix for calculation.
Line 271 changed the check for the return value of int parse_value(). That type of the function's return value is not of type enum sr_error_code. The main problem is, that the function always returns 0 (implicitly converted from SR_OK). The line 272 is never reached.
|
The "cleanup curly braces" leaves an empty // comment in line 73. The static function parse_value() that contains the actual fix is not called anywhere in the source code. It's dead code. The detection of ".0L" and similar strings is not present anymore. Is this intentional? |
src/dmm/vc96.c
Outdated
| @@ -264,6 +263,11 @@ SR_PRIV int sr_vc96_parse(const uint8_t *buf, float *floatval, | |||
|
|
|||
| memset(info_local, 0x00, sizeof(struct vc96_info)); | |||
|
|
|||
| if ((ret = parse_value(buf, floatval, &exponent)) != SR_OK) { | |||
| sr_dbg("Error parsing value: %d.", ret); | |||
There was a problem hiding this comment.
This program path can't be reached since the function parse_value always returns SR_OK,
|
I removed those nonreachable lines 266 and 267, they were for debugging only. |
|
Hi, another month is gone.
All request from harper23 were fixed.
What else has to done, to get this patch applied?
Matthias
…On Thu, 2023-11-30 at 23:42 -0800, harper23 wrote:
@harper23 commented on this pull request.
In src/dmm/vc96.c:
> @@ -264,6 +263,11 @@ SR_PRIV int sr_vc96_parse(const uint8_t *buf, float
> *floatval,
memset(info_local, 0x00, sizeof(struct vc96_info));
+ if ((ret = parse_value(buf, floatval, &exponent)) != SR_OK) {
+ sr_dbg("Error parsing value: %d.", ret);
This program path can't be reached sind the function parse_value always
returns S_OK,
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Hello sigrok,
I propose to add the changes to the sigrok repository.
I don't see any reason why this pull request should be rejected.
Best regards,
Helge
-------- Original Message --------
From: schuma-gta02 ***@***.***
Sent: Thursday, February 8, 2024 at 11:11 AM
To: sigrokproject/libsigrok
Cc: harper23; Mention
Subject: [sigrokproject/libsigrok] Update vc96.c (PR #234)
Hi, another month is gone.
All request from harper23 were fixed.
What else has to done, to get this patch applied?
Matthias
On Thu, 2023-11-30 at 23:42 -0800, harper23 wrote:
@harper23 commented on this pull request.
In src/dmm/vc96.c:
> @@ -264,6 +263,11 @@ SR_PRIV int sr_vc96_parse(const uint8_t *buf,
float
> *floatval,
memset(info_local, 0x00, sizeof(struct vc96_info));
+ if ((ret = parse_value(buf, floatval, &exponent)) != SR_OK) {
+ sr_dbg("Error parsing value: %d.", ret);
This program path can't be reached sind the function parse_value always
returns S_OK,
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID:
***@***.***>
—
Reply to this email directly, view it on GitHub
<#234 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABYESX3Z34AVZAHY6XCFCE3YSSQFHAVCNFSM6AAAAAA7ZKA2OKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZTG42TCMJRGM>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| @@ -63,25 +62,23 @@ static int parse_value(const uint8_t *buf, struct vc96_info *info, | |||
| is_ol += (!g_ascii_strcasecmp((const char *)&valstr, "-OL.")) ? 1 : 0; | |||
| is_ol += (!g_ascii_strcasecmp((const char *)&valstr, "-OL")) ? 1 : 0; | |||
| if (is_ol != 0) { | |||
| sr_spew("Over limit."); | |||
There was a problem hiding this comment.
What's the reason this was removed? Same question for all the other instances of sr_spew() that were removed.
|
Hi Soeren,
those were for debugging only and are not needed anymore.
Matthias
…On Mon, 2024-08-19 at 13:26 -0700, Soeren Apel wrote:
@abraxa commented on this pull request.
In src/dmm/vc96.c:
> @@ -63,25 +62,23 @@ static int parse_value(const uint8_t *buf, struct
> vc96_info *info,
is_ol += (!g_ascii_strcasecmp((const char *)&valstr, "-OL.")) ? 1 :
0;
is_ol += (!g_ascii_strcasecmp((const char *)&valstr, "-OL")) ? 1 : 0;
if (is_ol != 0) {
- sr_spew("Over limit.");
What's the reason this was removed?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
okay but it's SR_SPEW log level, which is perfectly adequate for this kind of verbosity and some messages should remain for debugging when the driver isn't working as intended. |
|
Do You want me to put them in again?
…On Mon, 2024-08-26 at 06:29 -0700, Soeren Apel wrote:
okay but it's SR_SPEW log level, which is perfectly adequate for this kind of
verbosity and some messages should remain for debugging when the driver isn't
working as intended.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I would prefer if you'd add them back in yeah, because it saves work for anyone trying to debug the driver in the future and no one runs libsigrok on SR_SPEW unless they really want to. |
reinvented sr_spew messages
|
Hi Soeren,
Done.
(To be honest: directly in the github Web UI.)
Matthias
…On Mon, 2024-08-26 at 06:47 -0700, Soeren Apel wrote:
I would prefer if you'd add them back in yeah, because it saves work for
anyone trying to debug the driver in the future and no one runs libsigrok on
SR_SPEW unless they really want to.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Fixes wrong detection and calculation.