Open mdavidsaver opened 6 years ago
I am trying to generate an error with the existing version of the code. I'm running the ADCSimDetector which generates NDArrays without the ColorMode attribute. In fact the NDArrays have no attributes at all. I then use NDPluginPva convert these to NTNDArrays which are sent to another IOC running pvaDriver. Both IOCs seem to be running fine with no errors. So although GetColorMode() should be failing I see no errors. How do I generate errors to know when I have fixed the problem?
Did you observe problems, or just notice the problem in the code?
So although GetColorMode() should be failing I see no errors. How do I generate errors to know when I have fixed the problem?
I also note that NTNDArray::getAttribute()
, as with most NTNDArray methods, returns NULL if the "attribute" field does not exists.
My guess then is that your Structure has "attributes", but that the PVStructureArray is empty.
To trigger a crash you would either have to get rid of "attributes" or change this StructureArray such that:
Really making the slightest change to the Structure definition will cause a crash.
Did you observe problems, or just notice the problem in the code?
I just noticed this. Incomplete error handling in the case of missing fields, or mis-matched types, is probably the most common gotcha of pvData.
If you want to experiment with different structure definitions have a look at iocBoot/iocimagedemo/ in QSRV. Removing the last two records "$(N):ColorMode_" and "$(N):ColorMode" will remove the "attributes" sub-structure.
https://github.com/epics-base/pva2pva/blob/master/iocBoot/iocimagedemo/image.db#L45
Similarly you can change the Field types by changing the record types.
Isn't attribute
a mandatory field of NTNDArray? If so, malformed NTNDArrays should be caught when wrapping them at the receiving code.
However, malformed attribute structures inside attribute
's array would cause the problem you mentioned.
@brunoseivam I have 2 IOCs that are generating NTNDArrays. ADSimDetector is generating arrays with several attributes including ColorMode, and ADCSimDetector is generating arrays with no attributes.
This is the output of pvget from ADSimDetector:
corvette:~>pvget -r 'field(attribute)' 13SIM1:Pva1:Image
13SIM1:Pva1:Image
structure
epics:nt/NTAttribute:1.0[] attribute
epics:nt/NTAttribute:1.0
string name ColorMode
any value
int 0
string[] tags []
string descriptor Color mode
int sourceType 0
string source Driver
epics:nt/NTAttribute:1.0
string name CameraManufacturer
any value
string Simulated detector
string[] tags []
string descriptor Camera manufacturer
int sourceType 2
string source 13SIM1:cam1:Manufacturer_RBV
epics:nt/NTAttribute:1.0
string name CameraModel
any value
string Basic simulator
string[] tags []
string descriptor Camera model
int sourceType 2
string source 13SIM1:cam1:Model_RBV
epics:nt/NTAttribute:1.0
string name ImageCounter
any value
int 9999
string[] tags []
string descriptor Array counter
int sourceType 2
string source 13SIM1:cam1:ArrayCounter_RBV
epics:nt/NTAttribute:1.0
string name MaxSizeX
any value
int 1936
string[] tags []
string descriptor Detector X size
int sourceType 2
string source 13SIM1:cam1:MaxSizeX_RBV
epics:nt/NTAttribute:1.0
string name MaxSizeY
any value
int 1216
string[] tags []
string descriptor Detector Y size
int sourceType 2
string source 13SIM1:cam1:MaxSizeY_RBV
epics:nt/NTAttribute:1.0
string name BinX
any value
int 1
string[] tags []
string descriptor Binning in X
int sourceType 2
string source 13SIM1:cam1:BinX_RBV
epics:nt/NTAttribute:1.0
string name BinY
any value
int 1
string[] tags []
string descriptor Binning in Y
int sourceType 2
string source 13SIM1:cam1:BinY_RBV
epics:nt/NTAttribute:1.0
string name AcquireTimePV
any value
double 0.001
string[] tags []
string descriptor Camera acquire time
int sourceType 2
string source 13SIM1:cam1:AcquireTime_RBV
epics:nt/NTAttribute:1.0
string name AcquirePeriod
any value
double 0.1
string[] tags []
string descriptor Acquire period
int sourceType 2
string source 13SIM1:cam1:AcquirePeriod_RBV
epics:nt/NTAttribute:1.0
string name ImageMode
any value
short 1
string[] tags []
string descriptor Image mode
int sourceType 2
string source 13SIM1:cam1:ImageMode_RBV
epics:nt/NTAttribute:1.0
string name TriggerMode
any value
short 0
string[] tags []
string descriptor Trigger mode
int sourceType 2
string source 13SIM1:cam1:TriggerMode_RBV
This is the output from pvget with ADCSimDetector:
corvette:~>pvget -r 'field(attribute)' 13ADCSIM1:Pva1:Image
13ADCSIM1:Pva1:Image
structure
epics:nt/NTAttribute:1.0[] attribute
So ADCSimDetector has no attributes, so there is no ColorMode attribute. But ntndArrayConverter is not generating any error. Is getColorMode() actually being called when converting NTNDArray to NDArray in pvaDriver? If so why is it not generating an error?
getColorMode
is only called from getInfo
, which in turn is called from here in pvaDriver.
If the underlying attribute
array is empty (but exists), getColorMode
's iterator will simply not do anything and the function will return a default value (NDColorModeMono
).
If the NTNDArray doesn't have the attribute
field, this error will (I expect) be caught here, by the wrap
method.
I think you will see an error if you purposefully generate an NTNDArray with an attribute
array containing bogus structures (e.g., missing the "name" field).
this error will (I expect) be caught here, by the wrap method.
Ah, were it only so... Actually it will crash here due to an unchecked getField() call (also returns NULL). Did I mention that this is a common error?
https://github.com/epics-base/normativeTypesCPP/blob/master/src/ntndarray.cpp#L352
To repeat my message. If you call a getSubField()
then you must check that the result is not NULL before using it. If you are certain it will never be NULL, then use getSubFieldT()
so that violations of this assumption will fail in a more obvious, and less destructive, way.
Also, the unchecked use of static_pointer_cast
is equally problematic.
What should be done here is something like
if(!*it) continue;
PVScalarPtr field((*it)->getSubFieldT<PVUnion>("value")->get<PVScalar>());
if(!field)
continue;
colorMode = field->getAs<uint32>();
This will throw if "value" doesn't exist, or isn't a union. It will silently ignore if "value" contains a non-scalar value (array or sub-structure). It will implicitly convert if the "value" union happens to contain PVShort or PVULong. It will throw if "value" contains a PVString with a value which can't be converted to an unsigned integer by the rules of castUnsafe<>()
(see pv/typeCast.h). But it should never crash.
edit: Added initial test of *it
as structure array elements may be NULL.
FYI You should also default to using PVScalar
or PVScalarArray
instead of eg. PVInt
or PVIntArray
. This adds some robustness in the face of what should be inconsequential changes in sign and width of integer types.
As efficient handling of array data is important to AD I will point out PVScalarArray::getAs<void>()
which allows access to raw array data as shared_vector<const void>
w/o copying.
Thanks for the tips, Michael! Is there any document with recommendations like this (or a more general "how to use pvData")? If not, is one planned?
Unfortunately no usage documentation exists yet which mentions PVScalar::getAs()/putFrom() or the void specialization of shared_vector. These are (somewhat) recent additions. It's in my mind to do this, but I don't yet have any time allocated.
FYI.
PVStructure::getSubField()
returns shared_ptr() (aka NULL) when the requested field doesn't exist. There is another variantPVStructure::getSubFieldT()
(notice the highly obvious "T") which throws an exception instead. When chaining method calls`getSubFieldT()
is highly recommended.https://github.com/areaDetector/ADCore/blob/acd97bd1f9241b3c0ae578d10156f254a4c893a6/ADApp/ntndArrayConverterSrc/ntndArrayConverter.cpp#L78