ChannelFinder / recsync

EPICS Record Synchronizor
Other
15 stars 25 forks source link

Add setReccasterEnvironmentVars iocsh function for custom env vars #57

Closed tynanford closed 1 year ago

tynanford commented 2 years ago

Per discussion here: https://github.com/ControlSystemStudio/epics-codeathon-2022/discussions/12#discussioncomment-2733048

This would add the ability to for a user to send their own environment variables in addition to the envs list here: https://github.com/ChannelFinder/recsync/blob/master/client/castApp/src/dbcb.c#L13

So you could call this in st.cmd:

setReccasterEnvironmentVars("SECTOR,BUILDING,CONTACT")

A code review and suggestions would be greatly appreciated. Thanks

kasemir commented 2 years ago

Combined with the recent addition of RSRV_SERVER_PORT https://github.com/epics-base/epics-base/pull/269, this allows us to track the TCP connection info of the IOC in the channel finder, which in turn enables building a 'name server'.

shroffk commented 2 years ago

@mdavidsaver could you have a look?

simon-ess commented 2 years ago

In general I wonder if a better idea would be to append the variables to the pre-generated list of environment variables to pass along. Thus we could use a syntax more akin to

addReccasterEnvironmentVar("SECTOR")
addReccasterEnvironmentVar("BUILDING")
addReccasterEnvironmentVar("CONTACT")

This should allow the send to not require special code to handle the extra variables. That said, this may have been discussed at the prior code-a-thon.

(This would also avoid re-parsing the customEnvsDefinition each time, but that's probably premature optimisation)

simon-ess commented 2 years ago

As per my suggestion above, here is one possible implementation: https://github.com/tynanford/recsync/pull/1

tynanford commented 2 years ago

thanks I like the change. Per @mdavidsaver we need to move the iocsh function definition to castinit.c and then we don't have to register addReccasterEnvironmentVar separately. I have that working with setReccasterEnvironmentVars, can update the branch with that for addReccasterEnvironmentVar

tynanford commented 2 years ago

@mdavidsaver @simon-ess made progress on placing addReccasterEnvironmentVar in castinit.c (https://github.com/tynanford/recsync/tree/update)

Now I see this memset which clears the caster_t struct during casterInit:

https://github.com/ChannelFinder/recsync/blob/master/client/castApp/src/caster.c#L111

So calling addReccasterEnvironmentVar works if called after iocInit but if called before iocInit then thecaster->extra_envs is null. Any tips on how I should go about this?

mdavidsaver commented 2 years ago

Any tips on how I should go about this?

I think that the call to casterInit() could be moved out of casthook() and into reccasterRegistrar() without ill effect.

tynanford commented 2 years ago

I think that the call to casterInit() could be moved out of casthook() and into reccasterRegistrar() without ill effect.

Thanks, that works well.

@mdavidsaver @simon-ess this PR is ready for a review again

Also BTW, I updated the default env list to include RSRV_SERVER_PORT and PVAS_SERVER_PORT as per our discussion last week. With base R7.0.7 I see RSRV_SERVER_PORT make it's way into CF

mdavidsaver commented 2 years ago

fyi. https://github.com/epics-base/pvAccessCPP/pull/185

simon-ess commented 1 year ago

Is there any progress on getting this merged in? This is functionality that we could happily use, but it seems to currently be sitting in limbo.

tynanford commented 1 year ago

Hi Simon, I will be at the code-a-thon at Diamond in a few weeks and was hoping to discuss this with yourself and @mdavidsaver or others if you guys will not be there. At this point I am waiting on a code review

tynanford commented 1 year ago

@mdavidsaver ready for a review

tynanford commented 1 year ago

Thanks for the review @mdavidsaver . Latest commit updates to use the argv++; argc--; to skip function name and uses the validation logic you mentioned to keep things in one loop. Could you take another look?

tynanford commented 1 year ago

@mdavidsaver how does that look?