Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/faebryk/core/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
logger = logging.getLogger(__name__)


# TODO: should this be a FaebrykException?
# TODO: should this include node and field information?
class FieldError(Exception):
pass

Expand Down Expand Up @@ -110,6 +112,13 @@ def __init__(self, node: "Node", *args: object) -> None:
self.node = node


class FieldConstructionError(FaebrykException):
def __init__(self, node: "Node", field: str, *args: object) -> None:
super().__init__(*args)
self.node = node
self.field = field


class NodeAlreadyBound(NodeException):
def __init__(self, node: "Node", other: "Node", *args: object) -> None:
super().__init__(
Expand Down Expand Up @@ -291,7 +300,7 @@ def append(name, inst):

return inst

def setup_field(name, obj):
def _setup_field(name, obj):
def setup_gen_alias(name, obj):
origin = get_origin(obj)
assert origin
Expand Down Expand Up @@ -321,6 +330,16 @@ def setup_gen_alias(name, obj):

raise NotImplementedError()

def setup_field(name, obj):
try:
_setup_field(name, obj)
except Exception as e:
raise FieldConstructionError(
self,
name,
f'An exception occurred while constructing field "{name}"',
) from e

nonrt, rt = partition(lambda x: isinstance(x[1], rt_field), clsfields.items())
for name, obj in nonrt:
setup_field(name, obj)
Expand Down
64 changes: 36 additions & 28 deletions src/faebryk/exporters/visualize/interactive_graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,16 @@
from faebryk.core.graphinterface import GraphInterface
from faebryk.core.link import Link
from faebryk.core.node import Node
from faebryk.exporters.visualize.util import IDSet, generate_pastel_palette
from faebryk.exporters.visualize.util import (
generate_pastel_palette,
offer_missing_install,
)
from faebryk.libs.util import FuncSet


def interactive_graph(G: Graph):
offer_missing_install("dash_cytoscape")
offer_missing_install("dash")
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

import dash_cytoscape as cyto
from dash import Dash, html

Expand Down Expand Up @@ -48,7 +54,7 @@ def _node(node: Node):
return {"data": data}

link_types: set[str] = set()
links_touched = IDSet[Link]()
links_touched = FuncSet[Link]()

def _link(link: Link):
if link in links_touched:
Expand Down Expand Up @@ -105,17 +111,42 @@ def _not_none(x):
},
]

def _pastels(iterable):
return zip(iterable, generate_pastel_palette(len(iterable)))
def _pastels(iterable, offset=0, spare=0):
return zip(
iterable,
generate_pastel_palette(len(iterable) + offset + spare)[
offset : -spare or None
],
)

for node_type, color in _pastels(node_types):
print("Node types:")
for node_type, color in _pastels(node_types, spare=len(node_types)):
stylesheet.append(
{
"selector": f'node[type = "{node_type}"]',
"style": {"background-color": color},
}
)

colored_text = rich.text.Text(f"{node_type}: {color}")
colored_text.stylize(f"on {color}")
rich.print(colored_text)
print("\n")

print("Link types:")
for link_type, color in _pastels(link_types, offset=len(node_types)):
stylesheet.append(
{
"selector": f'edge[type = "{link_type}"]',
"style": {"line-color": color, "target-arrow-color": color},
}
)

colored_text = rich.text.Text(f"{link_type}: {color}")
colored_text.stylize(f"on {color}")
rich.print(colored_text)
print("\n")

stylesheet.append(
{
"selector": 'node[type = "group"]',
Expand All @@ -128,14 +159,6 @@ def _pastels(iterable):
}
)

for link_type, color in _pastels(link_types):
stylesheet.append(
{
"selector": f'edge[type = "{link_type}"]',
"style": {"line-color": color, "target-arrow-color": color},
}
)

container_style = {
"position": "fixed",
"display": "flex",
Expand Down Expand Up @@ -193,19 +216,4 @@ def _pastels(iterable):
],
)

# print the color palette
print("Node types:")
for node_type, color in _pastels(node_types):
colored_text = rich.text.Text(f"{node_type}: {color}")
colored_text.stylize(f"on {color}")
rich.print(colored_text)
print("\n")

print("Link types:")
for link_type, color in _pastels(link_types):
colored_text = rich.text.Text(f"{link_type}: {color}")
colored_text.stylize(f"on {color}")
rich.print(colored_text)
print("\n")

app.run()
64 changes: 45 additions & 19 deletions src/faebryk/exporters/visualize/util.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,49 @@
import colorsys
from typing import Iterable, Sequence, Set
import importlib
import subprocess
import sys
from types import ModuleType

from rich.prompt import Confirm


class InstallationError(Exception):
"""Raised when there's a problem installing a module."""


def offer_install(module_name, install_name=None, ex=None) -> ModuleType | None:
"""
Offer to install a missing module using pip.
"""
cmd = [sys.executable, "-m", "pip", "install", install_name or module_name]

print(f"The module '{module_name}' is not installed.")

if Confirm.ask(
f"Do you want to run the install command [cyan mono]`{' '.join(cmd)}`[/]"
):
try:
# Attempt to install the module using pip
subprocess.check_call(cmd)

except subprocess.CalledProcessError:
print(f"Failed to install {module_name}. Please install it manually.")
raise ex or InstallationError(f"Failed to install {module_name}")

print(f"Successfully installed {module_name}")
return importlib.import_module(module_name)


def offer_missing_install(
module_name: str, install_name: str = None
) -> ModuleType | None:
"""
Offer to install a missing module using pip.
"""
try:
return importlib.import_module(module_name)
except ModuleNotFoundError:
return offer_install(module_name, install_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This is handy! Even though we should really make sure the deps are correct in the pyproject.
  • Since this uses pip directly and not poetry there might happen some unexpected versioning problems. It's probably good to put the dep into a group and use poetry to install it for optional deps.
  • Let's move this into libs/installation.py, I expect more of this to become relevant for usability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.



def generate_pastel_palette(num_colors: int) -> list[str]:
Expand Down Expand Up @@ -32,21 +76,3 @@ def generate_pastel_palette(num_colors: int) -> list[str]:
palette.append(hex_color)

return palette


# TODO: this belongs elsewhere
class IDSet[T](Set[T]):
def __init__(self, data: Sequence[T] | None = None):
self._data = set(data) if data is not None else set()

def add(self, item: T):
self._data.add(id(item))

def __contains__(self, item: T):
return id(item) in self._data

def __iter__(self) -> Iterable[T]:
return iter(self._data)

def __len__(self) -> int:
return len(self._data)
4 changes: 4 additions & 0 deletions src/faebryk/library/KicadFootprint.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
class KicadFootprint(F.Footprint):
def __init__(self, kicad_identifier: str, pin_names: list[str]) -> None:
super().__init__()
assert ":" in kicad_identifier, (
'kicad_identifier must be in the format "library:footprint".'
" If not, it'll cause downstream problems"
)

unique_pin_names = sorted(set(pin_names))
self.pin_names_sorted = list(enumerate(unique_pin_names))
Expand Down
51 changes: 51 additions & 0 deletions src/faebryk/library/NetTie.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# This file is part of the faebryk project
# SPDX-License-Identifier: MIT

import logging
from enum import Enum

import faebryk.library._F as F
from faebryk.core.module import Module
from faebryk.core.moduleinterface import ModuleInterface
from faebryk.library.Electrical import Electrical
from faebryk.library.KicadFootprint import KicadFootprint
Comment on lines +10 to +11
Copy link
Contributor

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.

Copy link
Contributor Author

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

from faebryk.libs.picker.picker import has_part_picked_remove

logger = logging.getLogger(__name__)


class NetTie[T: ModuleInterface](Module):
class Size(float, Enum):
_2_0MM = 2.0
_0_5MM = 0.5

unnamed: list[T]

def __init__(
self,
width: Size,
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

@iopapamanoglou iopapamanoglou Sep 6, 2024

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 _()

interface_type: type[T] = Electrical,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a net tie for non-electrical?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make a tree (maybe as a build artifact) of the types. It's non-obvious that Electrical is indeed the baseclass of everything a switch might be

) -> None:
super().__init__()

Copy link
Contributor

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): ...

Copy link
Contributor Author

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?

Copy link
Contributor

@iopapamanoglou iopapamanoglou Sep 6, 2024

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

# dynamically construct the interfaces
self.unnamed = self.add([interface_type(), interface_type()], "unnamed")
Copy link
Contributor

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

Copy link
Contributor

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")


# add dem trairs
self.add(F.can_bridge_defined(*self.unnamed))

width_mm = NetTie.Size(width).value

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"]
)
)
)
Comment on lines +39 to +51
Copy link
Contributor

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"]
                )
            )

1 change: 1 addition & 0 deletions src/faebryk/library/_F.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
from faebryk.library.Capacitor import Capacitor
from faebryk.library.Fuse import Fuse
from faebryk.library.Inductor import Inductor
from faebryk.library.NetTie import NetTie
from faebryk.library.Resistor import Resistor
from faebryk.library.Switch import Switch
from faebryk.library.B4B_ZR_SM4_TF import B4B_ZR_SM4_TF
Expand Down
20 changes: 18 additions & 2 deletions src/faebryk/library/has_multi_picker.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@

import logging
from abc import abstractmethod
from typing import Callable, Mapping
from typing import Callable, Mapping, Self

import faebryk.library._F as F
from faebryk.core.module import Module
from faebryk.core.node import Node
from faebryk.core.trait import TraitImpl
from faebryk.libs.picker.picker import PickError
from faebryk.libs.picker.picker import PickError, has_part_picked_remove

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -84,3 +84,19 @@ def handle_duplicate(self, other: TraitImpl, node: Node) -> bool:
other.pickers.extend(self.pickers)
other.pickers.sort(key=lambda x: x[0])
return False

@classmethod
def remove_if[T: Module](
cls, m: T, condition: Callable[[T], bool], prio: int = -10
) -> Self:
def replace(module: Module):
assert module is m
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those captures inhibit python from caching anonymous functions
Not really a perf bottleneck here, just wanted to note


if condition(module):
module.add_trait(has_part_picked_remove())

raise PickError("", m)
Comment on lines +95 to +98
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return in if missing


m.add(has_multi_picker(prio, has_multi_picker.FunctionPicker(replace)))

return m
Loading