search

ipbus / ipbus-firmware

Firmware that implements a reliable high-performance control link for particle physics electronics, based on the IPbus protocol
https://ipbus.web.cern.ch
Other
39 stars 31 forks source link

Missing 'id' attribute in opencores_i2c.xml #162

Closed jhegeman closed 4 years ago

jhegeman commented 4 years ago

The address table file for the OpenCores I2C master misses the top-level 'id' attribute. This is flagged by some of the address table parsing code, but does not seem to be a real problem. Still would be nice to fix though.

jhegeman commented 4 years ago

Corresponding pull request: #163.

jhegeman commented 4 years ago

Tom explained to me that this may be a feature instead of a problem, because the 'id' attribute in the top-level nodes of included address tables is never used.

In that case my suggestion would be to rigorously remove these attributes everywhere and to make the parser issue an warning/error in case they accidentally exist.

Tom is thinking about which way to go with this one.

tswilliams commented 4 years ago

Hi

On Tuesday, I created a new software release - v2.7.2 - in which I removed the spurious error message that I think you alluded to in the description of this ticket - i.e:

02-03-20 19:06:03.748014 [7f8609cf9740] ERROR - Failed to get attribute "id" from XML node.

Apologies for not fixing that sooner, but when we spoke a couple of weeks I somehow didn't fully realise at the time that a change in log messages led to this ticket (I must have missed the middle sentence when reading this issue's description as well).

I'll remove root node id attributes from the examples in both FW and SW repos, and update uHAL to print a log warning/error message when it finds the id attribute defined in the root node of an address table file.

Cheers, Tom

jhegeman commented 4 years ago

Hi @tswilliams,

The same actually holds for the 'id' attribute all the way at the top of the tree. The top node of the address table also has an 'id' attribute, but this is never used.

Cheers, Jeroen

tswilliams commented 4 years ago

Very true. This should be resolved in pull request #170

Cheers, Tom