search

leonardt / fault

A Python package for testing hardware (part of the magma ecosystem)
BSD 3-Clause "New" or "Revised" License
41 stars 13 forks source link

Add flatten method to mixed-signal types #231

Closed sgherbst closed 4 years ago

sgherbst commented 4 years ago

This is a re-submission of PR #230 since I was not able to push to the forked repo from @standanley. I just added the flatten method to the ElectType in addition to RealType to get all of the tests passing. In addition, I bumped the version to 3.0.11.

rsetaluri commented 4 years ago

@sgherbst thanks for fixing this; sorry, it was probably my fault that this wasn't checked when magma code was updated.

But it looks like these types are pretty much identical to magma's Digital type. Can we just inherit from that so we get all this for free? Looks like duplicated code. cc @leonardt

sgherbst commented 4 years ago

Thanks for the feedback -- I'm working on implementing it.

sgherbst commented 4 years ago

OK looks like the tests are passing. @rsetaluri Thanks again for the suggestion to subclass Digital -- that allowed for a net reduction of 138 lines of code! Would you mind reviewing the PR when you have a chance?

leonardt commented 4 years ago

Are these digital (and not analog) types? Perhaps we should have some base type of digital (e.g. Signal) that can be used? I Think that was the original reason for separating them (also it used to be _Bit, so maybe back then we didn't think about sharing the code paths).

sgherbst commented 4 years ago

That would be great, but it sounds like a change to magma right? For now, I wonder if we should merge this change because the master branch of fault is not passing CI tests. Then, when a signal type Analog is available from magma, we just need to replace Digital with Analog in fault/ms_types.py.

leonardt commented 4 years ago

yea it would be, seems like a reasonable path forward