Re: A question regarding sequencer compatibility

[Benjamin Franksen <benjamin.franksen@helmholtz-berlin.de>]

Hi Lewis & everyone else who answered

tl;dr the majority of commentors seem to think the breakage on the C 
side due to an extra timeout argument is manageable, so I'll probably 
just go ahead and add it.

For the multi-PV-array behaviour I'm considering a compiler switch, 
thanks to Tim's suggestion: turned off (default) = use with a MPVA gives 
an error, turned on = the new behaviour i.e. "forall PVs do...". This 
gives you a clean upgrade path: without the switch you can find all the 
places that might need to be fixed, and when you're done with that you 
can turn the switch on and profit from the better functionality.

> I'm OK with this change being made in 2.2.  However, why couldn't
> you use varargs to avoid needing to change escaped C code calls to
> seq_Pv{Put,Get}?  Or are you avoiding that to keep it clean and
> simple?

I've been thinking about a vararg solution but apart from the ugliness I 
believe it won't work in this case. With varargs you must be able to 
determine the types and values of all actual arguments at runtime 
(usually by inspecting one of the fixed arguments). When you want to add 
an optional argument this is exactly the information that you do /not/ 
have.

However, there is one trick I haven't yet mentioned: I could re-define 
the meaning of the existing optional argument, which is now of type

	enum compType {
		DEFAULT,
		SYNC,
		ASYNC
	};
	epicsShareFunc pvStat seq_pvGet(SS_ID, VAR_ID, enum compType);

into a tagged union:

	/* this would be in seqCom.h */

	enum compType {
		CT_DEFAULT,
		CT_SYNC,
		CT_ASYNC
	};

	struct compType {
	/* private: */
		enum compType tag;
		/* there is only one variant with data, so the union
		   and struct are redundant, they are only here for
		   documentation purposes */
		union {
			struct {
				double tmo;
			} sync;
		} data;
	};

	/* constructors, must remember to #ifdef the __inline__ for
	   non-gnu compilers... */
	__inline__ seq_ctDefault(void) {
		struct compType ct; ct.tag = CT_DEFAULT; return ct;
	}
	__inline__ seq_ctSync(double tmo) {
		struct compType ct; ct.tag = CT_SYNC; ct.data.sync.tmo = tmo; 
return ct;
	}
	__inline__ seq_ctAsync(void) {
		struct compType ct; ct.tag = CT_ASYNC; return ct;
	}

	#define DEFAULT seq_ctDefault()
	#define SYNC seq_ctSync(10.0)
	#define ASYNC seq_ctAsync()

	epicsShareFunc pvStat seq_pvGet(SS_ID, VAR_ID, struct compType);

If doing this stuff in C(90)[*] wouldn't be so extremely verbose I'd 
actually prefer this solution, as it makes clear (to humans /and/ to the 
type checker) that the timeout is only valid for synchronous operations.

If you think this is overkill, I agree; just wanted to mention it as a 
possibility. It's not 100% compatible either, I have seen lots of code 
where 0 is used in escaped C code to get the default behaviour instead 
of DEFAULT, so this code would then break.

Cheers
Ben

[*] it would be slightly less ugly in C99 or C++, with the latter we 
could even use optional arguments directly... in Haskell it would a 
matter of replacing

	data CompType = Default | Sync | Async

with

	data CompType = Default | Sync Double | Async

and that'd be all you need to change. (Sorry for the off-topic remark.)

Am Dienstag, 1. Oktober 2013, 12:19:58 schrieb J. Lewis Muir:
> On 10/1/13 9:48 AM, Benjamin Franksen wrote:
> > My current plan is therefore to give a compile time error if pv
> > functions are called with multi-PV arrays, as they do not properly
> > support them anyway. This means the compiler helps you by pointing
> > to
> > the location in your source code where a change is needed. Either
> > you
> > append "[0]" to the variable if that is actually the intention. Or
> > it's not, and as a result a few errors will be uncovered and
> > wouldn't
> > that be nice?[*]
> > 
> > What to do with pvPutComplete? It is the only function that properly
> > supports multi-PV arrays. I think I will remove this feature so that
> > it behaves in manner consistent with pvGetComplete, and then add a
> > new function that implements the more general behaviour. Again,
> > this means you might have to slightly change your program but the
> > compiler will help you in figuring out what to do.
> > 
> > BTW, on the subject of how to name the new functions (no
> > bike-shedding ensued, are you all asleep?) I am currently in favour
> > of using pural form i.e. pvGets, pvPuts, pvGetsComplete, etc. I
> > don't like pvGetArray since (1) the normal pvGet works perfectly
> > fine with arrays (only not with multi-PV arrays) and (2) the new
> > function will work with single-PV variables, too.
> 
> Hi, Ben.
> 
> I'm OK with your proposed changes, and I agree with your reasoning for
> favoring the plural naming over the "array" naming.
> 
> > And now to another issue: Currently, the timeout for synchronous
> > pvGet and pvPut is hard-coded to 10 seconds. Adding an extra
> > (optional) argument to specify a different timout has been on my
> > wish-list for a long time. This is now implemented (in the 2.2
> > branch) but again an issue of compatibility comes up, though in
> > this case it does not concern SNL code proper (adding an extra
> > DEFAULT_TIMEOUT argument can be done automatically by the
> > compiler). However, escaped C code that calls seq_Pv{Put,Get} will
> > have to be changed to add this argument. Note that here, too, the
> > compiler (the C compiler in this case) poinpoints the location of
> > the problem for you.
> > 
> > I am in favour of making this change in version 2.2, but if it turns
> > out to hurt people too much I am willing to restrict the change to
> > the new pvGets and pvPuts.
> 
> I'm OK with this change being made in 2.2.  However, why couldn't
> you use varargs to avoid needing to change escaped C code calls to
> seq_Pv{Put,Get}?  Or are you avoiding that to keep it clean and
> simple?
> 
> Lewis
-- 
"Make it so they have to reboot after every typo." -- Scott Adams

Attachment: signature.asc
Description: This is a digitally signed message part.