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

Add additional handling for spool weights #339

Merged
merged 19 commits into from
Apr 24, 2024

Conversation

spyder007
Copy link
Contributor

These are changes to implement methodology described in #338 as well as address the following feature requests:

#307 - Added empty_spool_weight as an optional field on vendor. This field is used as the default value for filaments saved from this vendor.
#299 - Added initial_weight and empty_weight to the spool object. These fields are defaulted from values on the filament, if they exist, but allow the user to set a higher initial weight than the filament + empty_weight allows.
#292 - Adding initial_weight effectively moves weight to the spool level. Users can ignore/not set weight at the filament level and then create spools with initial weights.
#287 - I added a measure endpoint on the spool which allows you to pass in the spool's gross weight. Based on the initial weight of the spool, it's empty weight, and the current usage on the spool, a new usage is calculated.

I did not modify any of the existing integration tests to ensure that old functionality would still work. I added new tests to cover the new fields and functionality.

Note there are a few new strings. I added to the English set, but they would need translated.

@spyder007
Copy link
Contributor Author

I added a change to add libffi-dev to the python-builder image... the arm7 builds were failing without it.

spoolman/database/spool.py Outdated Show resolved Hide resolved
@spyder007 spyder007 requested a review from Donkie April 8, 2024 16:31
@Donkie
Copy link
Owner

Donkie commented Apr 8, 2024

Would it make sense to make this better synchronized with the existing weight values in the Filament object? The initial_weight is gross weight, while the existing filament "weight" field is net weight. Also perhaps empty_weight could be renamed to spool_weight.

Copy link
Owner

@Donkie Donkie left a comment

Choose a reason for hiding this comment

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

Added some more comments. I haven't reviewed the client part yet

spoolman/api/v1/vendor.py Outdated Show resolved Hide resolved
Comment on lines 376 to 377
if weight > current_use:
raise SpoolMeasureError("Measured weight cannot increase.")
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it should be able to? I don't see why not. Then you could occasionally re-measure your spools and if there has been some discrepancy such that Spoolman thinks more filament has been used than it actually has, it can be corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now this function utilizes the use_weight function, as to not replicate this functionality. The original idea was that measure was just another flavor of use (like use weight or use length). If I allow for the measured weight to effectively add weight to the spool, it should probably do so on it's own logical path.

If we want to allow for measurements to add, we can, but how would you want to go about this? I'm not opposed, but I'd like to know how you would want that to look.

Copy link
Owner

Choose a reason for hiding this comment

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

use_weight supports negative numbers, so this restriction is completely arbitrary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue I was running in to was what happens if your measurement is greater than the initial weight. In this case, I just bump the initial weight to the measurement and set the used to 0.

spoolman/database/spool.py Outdated Show resolved Hide resolved
spoolman/database/spool.py Outdated Show resolved Hide resolved
tests_integration/tests/spool/test_measure.py Show resolved Hide resolved
spoolman/database/spool.py Outdated Show resolved Hide resolved
@spyder007
Copy link
Contributor Author

Would it make sense to make this better synchronized with the existing weight values in the Filament object? The initial_weight is gross weight, while the existing filament "weight" field is net weight. Also perhaps empty_weight could be renamed to spool_weight.

The issue is that the filament weight is, well, not really sensible: generally, a filament is a particular filament type. Why would I have two different filaments in the system that are "Sunlu Black Matte PLA" if I have a 1KG spool and a 3KG spool. The idea in putting the weights at the spool level was to allow filaments to be able to feed multiple spool sizes or types. Say I respool some of my filament from a 3KG roll to a roll that is, oh, say 750 KG just because that's what I had. Even when 1KG rolls come from the manufacturer, their gross weight can be a little more because they added some extra filament.

The general idea behind this change was to allow for that flexibility but not break existing functionality, which is why I'm grabbing the filament weight and spool weight on create. But after initial creation, the spool should use it's own initial weight if available.

I actually toyed with writing a conversion script to go through and default all existing spools, but that felt like it would break more than it fixed.

@spyder007 spyder007 requested a review from Donkie April 8, 2024 22:17
@Donkie
Copy link
Owner

Donkie commented Apr 9, 2024

Would it make sense to make this better synchronized with the existing weight values in the Filament object? The initial_weight is gross weight, while the existing filament "weight" field is net weight. Also perhaps empty_weight could be renamed to spool_weight.

The issue is that the filament weight is, well, not really sensible: generally, a filament is a particular filament type. Why would I have two different filaments in the system that are "Sunlu Black Matte PLA" if I have a 1KG spool and a 3KG spool. The idea in putting the weights at the spool level was to allow filaments to be able to feed multiple spool sizes or types. Say I respool some of my filament from a 3KG roll to a roll that is, oh, say 750 KG just because that's what I had. Even when 1KG rolls come from the manufacturer, their gross weight can be a little more because they added some extra filament.

The general idea behind this change was to allow for that flexibility but not break existing functionality, which is why I'm grabbing the filament weight and spool weight on create. But after initial creation, the spool should use it's own initial weight if available.

I actually toyed with writing a conversion script to go through and default all existing spools, but that felt like it would break more than it fixed.

I think you misunderstood me, I don't disagree with having these two fields, but I would just want you to change so initial_weight is net weight instead of gross weight, and change so empty_weight is called spool_weight. Then it's more aligned with how the weight fields are defined in the filament object, so as to reduce confusion when using the API. Gross weight can still be calculated using initial_weight + spool_weight.

@spyder007
Copy link
Contributor Author

Would it make sense to make this better synchronized with the existing weight values in the Filament object? The initial_weight is gross weight, while the existing filament "weight" field is net weight. Also perhaps empty_weight could be renamed to spool_weight.

The issue is that the filament weight is, well, not really sensible: generally, a filament is a particular filament type. Why would I have two different filaments in the system that are "Sunlu Black Matte PLA" if I have a 1KG spool and a 3KG spool. The idea in putting the weights at the spool level was to allow filaments to be able to feed multiple spool sizes or types. Say I respool some of my filament from a 3KG roll to a roll that is, oh, say 750 KG just because that's what I had. Even when 1KG rolls come from the manufacturer, their gross weight can be a little more because they added some extra filament.
The general idea behind this change was to allow for that flexibility but not break existing functionality, which is why I'm grabbing the filament weight and spool weight on create. But after initial creation, the spool should use it's own initial weight if available.
I actually toyed with writing a conversion script to go through and default all existing spools, but that felt like it would break more than it fixed.

I think you misunderstood me, I don't disagree with having these two fields, but I would just want you to change so initial_weight is net weight instead of gross weight, and change so empty_weight is called spool_weight. Then it's more aligned with how the weight fields are defined in the filament object, so as to reduce confusion when using the API. Gross weight can still be calculated using initial_weight + spool_weight.

So STORE net weight and tare weight as initial_weight and spool_weight, and only calculate gross weight as needed. I will make those changes.

@Donkie
Copy link
Owner

Donkie commented Apr 13, 2024

Client-side looks solid also, nice job :) Just a few more minor comments then we can merge this.

@spyder007 spyder007 requested a review from Donkie April 15, 2024 22:06
@Donkie Donkie merged commit fa4905d into Donkie:master Apr 24, 2024
12 checks passed
@spyder007 spyder007 deleted the feature/spool_revisions branch April 29, 2024 19:29
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.

None yet

2 participants