fix incorrect return value in M4i driver for ACDC_offs_compensation_x

[peendebak]

The ACDC_offs_compensation_x parameters did not return any value. This PR fixes this by returning the correct value

Also the warning that is generated when the card is in HF mode is reduced, since the card is by default not in HF mode and therefore the warning is invalid.

The warning is converted to info level and the docstring of the parameter is updated.

[codecov]

Codecov Report

Merging #1585 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1585   +/-   ##
=======================================
  Coverage   72.33%   72.33%           
=======================================
  Files         116      116           
  Lines       12388    12388           
=======================================
  Hits         8961     8961           
  Misses       3427     3427

[astafan8]

... The easiest way around this is to change the logging.warning into a logging.info

i don't think it is an "ok" solution. hiding the issue is not acceptable.

but what happens when HF is disabled and self._param32bit(getattr(pyspcm, 'SPC_ACDC_OFFS_COMPENSATION{}'.format(i))) is executed? does the pyspcm throw some error? i'm trying to understand the reason for the "if" statement.

---

generally, if existence and/or meaning of some parameters depends on whether HF is enabled or not, then the driver should've been implemented in such a way that these parameters only appear when HF is enabled. Perhaps, HF-related things should've been encapsulated in its own submodule/InstrumentChannel/whatever. But since it's not the case, and we seem to want a quick solution, i'd prefer to know first why the "if" statement is needed. And if it is indeed, needed, then i'd suggest to add some sort of "UNDEFINED" value to the list of allowed values of that parameter, such that the value of this parameter is UNDEFINED when HF is disabled.

another "work around" that i can suggest (although it's also not good) is to set "snapshot_get" of the parameter to True when HF is enabled and to False when HF is disabled. This will avoid re-getting the parameter value from the instrument upon snapshotting with update=True when HF is disabled.

[astafan8]

are you planning to fix the "warning issue" in this same PR? If so, after pushing the fix, please update the name and the description of the PR so that it's clear what is being changed and why. The PR descriptions are crucial when looking for something in the history, and for writing changelogs upon a new release.

[peendebak]

@astafan8 I don't think the original authors of the driver used these options, and neither do I so I don't know any reasons by the current code is the way it is.

I would say that converting the warning to info in the get is acceptable, if we keep the warning in the set.
Otherwise, I would prefer to keep the set functionality as is, but let the get return an UNDEFINED instead of generating a warning. (I think it makes sense to keep it an int, so I would pick UNDEFINED=-1 I think)

[astafan8]

... I think it makes sense to keep it an int, so I would pick UNDEFINED=-1 I think ...

magic numbers are never a good idea ;)

I would say that converting the warning to info in the get is acceptable

ok, but i then need to ask - if there is no warning on get, and HF is disabled, the "parameter get" will return None (according to the current implementation), and this fact will be unnoticed by the user until he/she wants to use the value of that parameter for "something", and that "something" breaks (or worse, behaves unexpectedly) because it doesn't expect to work with the value of None. Or is this NOT a concern for this particular parameter? If it's not a concern, ok, let's convert the warning in "get" into "info", and add information about this into the docstring of the "compensation" parameter (so that users know that "compensation" parameter should be used only when HF is enabled).

[peendebak]

@astafan8 To be honest, I don't even know what this parameter is. But each time a user connects to the M4i (with default values) we get a warning with the get statement, which doesn't make sense. The current behaviour of the get (e.g. always return None) is also wrong.
This PR makes sure at least the correct value is returned. I would also want to prevent the warning from happening when the driver is initialized (when doing a snapshot from the station).

It is the responsbility of the user to make sure the correct settings are used in HF mode. I am fine with adding a docstring and a logging.info to give the user proper hints.

[peendebak]

@astafan8 I updated the PR and the description

[astafan8]

( thanks for the updated description! )