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

Adding Assembly Creation and Rendering #89

Closed
wants to merge 3 commits into from
Closed

Conversation

jmwright
Copy link
Contributor

Changes will be pushed to this branch to get assembly creation and rendering to png working.

@jmwright
Copy link
Contributor Author

@julianstirling I fixed the one test that I had originally roughed out. The Nimble configuration class is instantiating a Shelf object, and so I am pulling the data back out of that instance to instantiate a RaspberryPiShelf object, which seems odd. Can the configuration class just call the new create_shelf_for_device method to get a shelf instead of instantiating a generic Shelf?

@julianstirling
Copy link
Collaborator

100% that was my plan. So the configuration class should just take the name it was asked for and pass that to create_shelf_for_device which should send it back the correctly classed shelf object.

To be fair it probably makes sense for the configuration class to just pass the device ID to create_shelf_for_device and get back the shelf object of the correct class. If the configuration class needs to get info from the device it should probably use shelf.device.xxx This way the configuration class doesn't need and extra list of the Device objects.

@jmwright
Copy link
Contributor Author

@julianstirling So should the Shelf classes not accept assembly_key, position, and color, and only accept the device ID as well? I would have to undo the changes you made to the last PR to make that happen.

@julianstirling
Copy link
Collaborator

Sorry, I meant just as in "just the device id string not the full Device class" rather than "only the id string and no other variables".

I'd think that:

def create_shelf_for_device(device_id: str, assembly_key:str, position:tuple[float,float,float], color:str):
    if device_id in raspi_ids:
        shelf_obj = RaspberryPiShelf(device_id, assembly_key, position, color)
    else:
        # Other shelf classes to be added
        shelf_obj = Shelf(device_id, assembly_key, position, color)

    return shelf_obj

Then the __init__.py method of Shelf could create the Device object from the device_id

@jmwright
Copy link
Contributor Author

I'm not sure I understand why you would recreate the device instance within Shelf.__init__ when it was already created by the configuration. Won't Shelf just have to import NimbleConfiguration to create the device again? Am I missing something about how Shelf will be called?

@julianstirling
Copy link
Collaborator

My thought was that it is cleaner to move the creation of the Device object from Configuration to Shelf. This way anything that says "I want a shelf for this" will get all the info they need.

I suppose the question is are we chasing our tails. My thought was that if something external can fire up a Shelf object with just a device ID string then that makes it easy for exsource to create the correct shelf object to get the model...

But now I see we also need to pass it the rack parameters object... However to me it makes sense for the shelf to know the rack parameters...

@jmwright
Copy link
Contributor Author

I suppose the question is are we chasing our tails.

I feel like I am. There is a chicken-and-egg problem with the Shelf class and the NimbleConfiguration class which will require me to make changes to code outside of my silo. I'm wondering if it would be best if you make the changes in a branch, and then I restart this PR from that branch and add the new CAD-related things to the Shelf class(es).

@jmwright
Copy link
Contributor Author

I'm closing this PR since it will be superceded by a later one.

@jmwright jmwright closed this Aug 21, 2024
@jmwright jmwright deleted the assembly-render branch August 22, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants