Feature Request: Parse Two XML Address Tables Instead of One

[bdorney]

Brief summary of issue

The CTP7 and OHv3a each have their own xml file which defines the address table. See for example:

https://github.com/evka85/GEM_AMC/releases/tag/v3.2.6 https://github.com/andrewpeck/OptoHybridv3/releases/tag/3.0.7.A

Right now for the XML to be parsed you have to copy/paste the OHv3a xml into the CTP7 xml which is a bit tedious.

Would like that parseXML() is changed to work with two files instead of one.

Types of issue

• [ ] Bug report (report an issue with the code)
• [X] Feature request (request for change which adds functionality)

Expected Behavior

Simply making the links (e.g. gem_amc_top.xml and gem_oh_top.xml) for the two xml's in the appropriate location parseXML() should use both of them.

If gem_oh_top.xml is not found it should print a warning but continue parsing the xml to be backwards compatible with older FW versions/v2b electronics.

Current Behavior

You have to copy/paste the OHv3a xml into the CTP7 xml.

Context (for feature requests)

Quality of life improvement.

[mexanick]

Could you please specify where exactly you insert the OHv3a table in the CTP7 at the moment? is it just before this line? https://github.com/evka85/GEM_AMC/blob/v3.2.6/scripts/address_table/gem_amc_top.xml#L3150

[bdorney]

Hi @mexanick I do not know. Last time it was done by @andrewpeck and @evka85. Maybe they could add some input here.

[mexanick]

ok, I talked to @evka85 and as fas as I understand the other XML file has to be included here:

https://github.com/evka85/GEM_AMC/blob/2c9d430ee60c4a79bdbea01c494b0588b20ebacb/scripts/address_table/gem_amc_top.xml#L1232-L1237

The only remaining issue is preserving the node hierarchy. I see an extra node with the name FPGA in @andrewpeck XML:

https://github.com/andrewpeck/OptoHybridv3/blob/9c3a3afb216d6c8ccee6c11291f7d56e1c7b34c6/optohybrid_registers.xml#L1-L7

I believe it should be removed.

Now, considering the syntax of inclusion: in order to preserve the compatibility with uHAL XML parser I suggest to use module attribute like here:

https://github.com/cms-gem-daq-project/cmsgemos/blob/91ab7d5bb582e20e2fb072822d0c5992dbd3cef8/setup/etc/addresstables/glib_address_table.xml#L6

[andrewpeck]

Hi Misha,

On Tue, Jan 23, 2018 at 5:49 AM, Mykhailo Dalchenko < notifications@github.com> wrote:

ok, I talked to @evka85 https://github.com/evka85 and as fas as I understand the other XML file has to be included here: https://github.com/evka85/GEM_AMC/blob/2c9d430ee60c4a79bdbea01c494b05 88b20ebacb/scripts/address_table/gem_amc_top.xml#L1232-L1237

The only remaining issue is preserving the node hierarchy. I see an extra node with the name FPGA in @andrewpeck https://github.com/andrewpeck XML: https://github.com/andrewpeck/OptoHybridv3/blob/ 9c3a3afb216d6c8ccee6c11291f7d56e1c7b34c6/optohybrid_registers.xml#L1-L7

It's a matter of perspective whether there is an extra node in the original file, or a missing node in the file it was copy pasted into :)

I would prefer that all of the OH FPGA registers were kept split into a separate node (FPGA)...

Right now the tree GEM_AMC.OH.OH0 contains other node (e.g. GEM_AMC.OH.OH0.GEB) mixed in with the OH FPGA registers

I think it would be much cleaner to retain the "FPGA" node so that it would look like:

GEM_AMC.OH.OH0.GEB GEM_AMC.OH.OH0.FPGA etc.

and keep a clear distinction of what the registers belong to.

Any thoughts on this from others?

Thanks,

Andrew

I believe it should be removed.

Now, considering the syntax of inclusion: in order to preserve the compatibility with uHAL XML parser I suggest to use module attribute like here: https://github.com/cms-gem-daq-project/cmsgemos/blob/ 91ab7d5bb582e20e2fb072822d0c5992dbd3cef8/setup/etc/ addresstables/glib_address_table.xml#L6

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-gem-daq-project/xhal/issues/34#issuecomment-359795987, or mute the thread https://github.com/notifications/unsubscribe-auth/AG5f8FIFYSCK61QIUEfVJiuHbpcbgmCGks5tNeNrgaJpZM4RmW29 .

[mexanick]

@andrewpeck, while I agree that it might be cleaner with the extra FPGA node, I should admit that it will break a lot of things and will require a number of fixes to the code pieces where the base name of registers has been hard coded (e.g. like https://github.com/cms-gem-daq-project/cmsgemos/blob/8681ba44985fc65dd6bf6f0bba295ca76459b5ec/gemhardware/src/common/optohybrid/HwOptoHybrid.cc#L887) @jsturdy @bdorney what you think? I personally would preserve the compatibility here...

[bdorney]

The link above generates a 404 error.

However for resolving hard coded errors in C++ you could have a struct that is defined in a given namespace. I am not sure how the namespaces go, but for example:

namespace GEM{
   namespace addressTable{
      struct FWBaseNames{
            std::string v2b;
            std::string v3;

            FWBaseNames(std::string strLink){
                 v2b = //the v2b base name;
                 v3 = "GEM_AMC.OH.OH" + strLink;
             }
        }
    }
}

Then an instance of FWBaseNames could be used everywhere and then if any future changes are needed it would only have to be done in one place (some like GEMUtilities.h file).

I agree with @andrewpeck that it is cleaner and would favor this. A similar solution for gempython and other repo's could also be implemented.

Of course this would require some code compatibility changes but maybe in the long run it would be easier to maintain (e.g. have one header file or imported file where it is set rather than multiple locations in the SW).

[jsturdy]

I don't think it would be that much work to accommodate the structure that @andrewpeck realized (which admittedly, is a long time coming 👍, as I think everyone here knows that I'm a big fan of semantic sensibility and correctness :-) )

It would simply be a matter of targeting a release and updating the code for that compatibility release (V2 and V3 concurrently), and releasing a simultaneous set of new address tables

[andrewpeck]

Hi Jared, Brian

Thanks. You made my day :)

I will enjoy being able to do a OH0.FPGA dump to get all of the registers I care about

Best,

Andrew

On Tue, Jan 23, 2018 at 8:06 AM, Jared Sturdy notifications@github.com wrote:

I don't think it would be that much work to accommodate the structure that @andrewpeck https://github.com/andrewpeck realized (which admittedly, is a long time coming 👍, as I think everyone here knows that I'm a big fan of semantic sensibility and correctness :-) )

It would simply be a matter of targeting a release and updating the code for that compatibility release (V2 and V3 concurrently), and releasing a simultaneous set of new address tables

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-gem-daq-project/xhal/issues/34#issuecomment-359839439, or mute the thread https://github.com/notifications/unsubscribe-auth/AG5f8KJ2LyKNYa5APH_UsoxoL9wTAofZks5tNgNsgaJpZM4RmW29 .

[mexanick]

hi @andrewpeck , with the latest PR I made you will have it =)

[bdorney]

Given that https://github.com/cms-gem-daq-project/xhal/pull/36 was merged I think this can be closed. Please re-open if you object.