-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bikey-Wagon Learnings #14
Conversation
27bbefd
to
ee10ce3
Compare
bd7c815
to
408c46d
Compare
408c46d
to
755aca5
Compare
Move FuncSet and FuncDict hashable collections to libs/util. Make colours for links and nodes unique.
9b27698
to
e263abb
Compare
Ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks for the most part good.
I guess the NetTie was made before fab ll?
interface_type: type[T] = Electrical, | ||
) -> None: | ||
super().__init__() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init in fab ll is purely for argument passing
split into init and preinit
def __init__(
self,
width: Size,
interface_type: type[T] = Electrical,
) -> None:
super().__init__()
self._interface_type = interface_type
self._width = width
def __preinit__(self): ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait though, if __preinit__
is called before __init__
then how can I access _width
during my static configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order is init, annotations, d_fields, rt_fields, preinit, postinit
I can see where the confusion comes from.
The pre/post refers to pre/post base class construction
self.add(F.can_attach_to_footprint_symmetrically()) | ||
self.add(F.has_designator_prefix_defined("H")) | ||
# TODO: "removed" isn't really true, but seems to work | ||
self.add(has_part_picked_remove()) | ||
|
||
# TODO: generate the kicad footprint instead of loading it | ||
self.add( | ||
F.has_footprint_defined( | ||
KicadFootprint( | ||
f"NetTie:NetTie-2_SMD_Pad{width_mm:.1f}mm", pin_names=["1", "2"] | ||
) | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static traits should be in the class body
attach_to_footprint: F.can_attach_to_footprint_symmetrically
designator_prefix = L.f_field(F.has_designator_prefix_defined)("H")
@L.rt_field
def footprint(self):
width_mm = NetTie.Size(self._width).value
return F.has_footprint_defined(
KicadFootprint(
f"NetTie:NetTie-2_SMD_Pad{width_mm:.1f}mm", pin_names=["1", "2"]
)
)
super().__init__() | ||
|
||
# dynamically construct the interfaces | ||
self.unnamed = self.add([interface_type(), interface_type()], "unnamed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to do the self.unnamed =
, it's done by self.add automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use times
:
self.add(times(2, self._interface_type), "unnamed")
from faebryk.library.Electrical import Electrical | ||
from faebryk.library.KicadFootprint import KicadFootprint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid direct imports from faebryk.library, use F.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pulling this into my project for now and separating the PRs.
Here's another draft for it specifically: #39
|
||
def __init__( | ||
self, | ||
width: Size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the width be a parameter instead of constructor argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, but then I'd need to defer construction until later to make the footprint trait work.
Until that's all working this seems simpler and clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need delayed construction, because you are not constructing anything. Adding a trait is always possible.
You can use is_implemented
in the trait to check whether the width is 'ready' for making a footprint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
width: TBD[Quantity]
...
@L.rt_field
def footprint(self):
class _(F.has_footprint):
@staticmethod
def get_footprint():
value = self.width.get_most_narrow()
assert isinstance(value, Constant)
width_mm = value ...
return F.KicadFootprint(f"NetTie:NetTie-2_SMD_Pad{width_mm:.1f}mm", pin_names=["1", "2"])
@staticmethod
def is_implemented(_self):
return isinstance(self.width.get_most_narrow(), Constant)
return _()
""" | ||
|
||
def __init__( | ||
self, data: Sequence[T] | None = None, hasher: Callable[[T], Hashable] = id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data: Iterable[T]
self._deref = {hasher(d[0]): d[0] for d in data} if data else {} | ||
self._data = {hasher(d[0]): d[1] for d in data} if data else {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment in FuncSet about multidict / hash uniqueness
def __contains__(self, item: T): | ||
return self._hasher(item) in self._deref | ||
|
||
def __iter__(self) -> Iterable[T]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think iter returns Iterator[T], not sure tho
test/libs/util_func_collections.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thank you added tests!
I'd add one for non default hash func that has collisions: e.g `lambda x: id(x) % 10'
offer_missing_install("dash_cytoscape") | ||
offer_missing_install("dash") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in visualize.
Having a poetry dep group for this and then check if its there and if not print: install via poetry ...
might be cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this one's a little weird. Not sure I love the offer_missing
pattern for the deps at all.
Separately, we should shoot for a vanilla pip
/pipx
install working by default, not poetry
. Let's detangle that where when we come by it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik currently everything works with a pip only setup
Most things here merged in different PRs |
Commit per change
Checklist
Please read and execute the following:
Code of Conduct
By submitting this issue, you agree to follow our Code of Conduct: