Update yml and interface #12
Conversation
|
The requested changes were updated in the latest commits. LGTM! @bruno-f-cruz do you agree? |
| @@ -17,7 +17,7 @@ | |||
| <PackageLicenseFile>LICENSE</PackageLicenseFile> | |||
| <PackageOutputPath>..\bin\$(Configuration)</PackageOutputPath> | |||
| <TargetFrameworks>net462;netstandard2.0</TargetFrameworks> | |||
| <VersionPrefix>0.1.0</VersionPrefix> | |||
| <VersionPrefix>0.1.1</VersionPrefix> | |||
There was a problem hiding this comment.
| <VersionPrefix>0.1.1</VersionPrefix> | |
| <VersionPrefix>0.2.0</VersionPrefix> |
Feels like this should be a bump to the minor version since new registers and features were added, no?
There was a problem hiding this comment.
The rational was based on @bruno-f-cruz comment: " I think that prior to 1.x, we tend to use the patch from semver to indicate backward compatible. If these are indeed fully backwards compatible i would tag this release as 0.1.1."
There was a problem hiding this comment.
Uhm my bad I think I am confused. I don't know why I thought this was the agreement during the 0.x but maybe I am confused with the yml firmware version that only allows 2 numbers to be represented. Sorry!
There was a problem hiding this comment.
Sounds great, in that case @artursilva0 changing this version to 0.2.0 and the name of the calibration register are the only two changes needed, everything else LGTM!
| type: U8 | ||
| access: Read | ||
| description: Enable flow adjustment based on the temperature calibration. | ||
| UserTemperatureCalibration: |
There was a problem hiding this comment.
What is the difference between TemperatureValue and UserTemperatureCalibration? Is this a reference value used to establish the adjustment automatically?
I am not very fond of adding the general term User here, since a "user" is very vaguely defined (and could be automated away in the future). It would be more helpful to describe here what exactly is expected this value to contain, e.g. if this is a reference value, we might consider changing this to ReferenceTemperatureValue and the description to more explicitly indicate what is the meaning of the values provided here.
There was a problem hiding this comment.
TemperatureValue is the actual value of the device temperature
UserTemperatureCalibration is used for calibration purposes only if the user decides to calibrate the device. This temperature calibration value is the value at which the calibration was done.
User is used to be consistent with former register description used for the calibration of the device, when performed by the user (the device always contains the factory calibration values, so User is used to distinguish this).
There was a problem hiding this comment.
TemperatureCalibrationValue would be more clear?
There was a problem hiding this comment.
TemperatureCalibrationValue would be more clear?
Definitely, that would be great if it works!
There was a problem hiding this comment.
It should be noted that adding the word User would make some sense given the current interface (
device.olfactometer/device.yml
Line 38 in 621022c
Should we break this while we are still in 0.x?
There was a problem hiding this comment.
In fact, I've double checked and this register is shared for both the user and the factory calibration, so in the end TemperatureCalibrationValue seems more appropriated. Do you agree @bruno-f-cruz , so that I can make the mods and merge these files?
Update yml and interface to add a new feature which allows temperature compensation on the flow rate for proportional valves with temperature sensors (back compatibility assured). Reserved registers were renamed and used for the new registers added, so I don't think that there is anything breaking, but please confirm.