AMI430 driver: safety around coil_constant and ramp limits
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 459
- Forks
- 359
- Avg merge
- 3d 6h
- Merged PRs (30d)
- 73
Description
I want to change the physical coils of the magnet, and I would like to update the safety limits accordingly.
I use the code:
```
ami_yellow.current_ramp_limit(0.1762) # A/s
ami_yellow.ramp_rate(0.1762 * 0.01490) # T/s <<<-- exception is raised here
ami_yellow.coil_constant(0.01490) # T/A
ami_yellow.current_limit(67.11) # A
```
(unfortunately traceback is lost, but the exception originated from `_set_ramp_rate` method, it's the ValueError that that method may raise (see line 369 of AMI430.py).)
It seems that the reason for this exception is that field_ramp_limit was not updated when I set current_ramp_limit above. Hence, the driver thinks that my ramp rate is out of bounds even though it is not (because i set the current_ramp_limit before).
Then, i tried the following:
```
ami_yellow.current_ramp_limit(0.1762) # A/s
ami_yellow.field_ramp_limit(0.1762 * 0.01490) # T/s <<<-- NOTE that I ALSO set field_ramp_limit parameter this time
ami_yellow.ramp_rate(0.1762*0.01490) # T/s
ami_yellow.coil_constant(0.01490) # T/A
ami_yellow.current_limit(67.11) # A
```
And this time there was no exception raised.
However, setting field_ramp_limit seems to overwrite my current_ramp_limit (because coil constant has not updated yet, i.e. it is not "0.01490" yet but some other previous value), so current_ramp_limit apparently gets set to a value which is out of my safety limits.
My proposed solution is a method of setting all safety limits and constants in a single function, and sending those to the instrument in a single call, preferebly in __init__.
Moreover, I would even propose to make the coil_constant, current_ramp_limit (and thereby field_ramp_limit) and current_limit to unchangable after __init__ (so that when coils are changed, the user has to reinstantiate the magent driver).
Along the way found the following bug:
* scale of field_ramp_limit (which is dependent on the coil constant) is fixed in __init__, and this will bring problems if users change coil constanst.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Review AMI430.py, especially __init__ and _set_ramp_rate around line 369, alongside the coil_constant, current_ramp_limit, field_ramp_limit, and current_limit setters. Confirm how these values interact when configuring a changed coil, then define completion as safety limits remaining consistent without the reported exception or unintended overwrite.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100