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 features: A combination of other PRs + Making the API more flexible. #24

Closed
wants to merge 58 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented May 13, 2024

There is a very similar PR open already here #22, in fact I just copied one of the changes made in there and updated it further.

I opened this PR because the original did not get merged due to other changes unrelated to this one.

The feature is quite simple, the possibility of setting the integral sum term manually. This is necessary for restoring the state of the controller after a shutdown, as discussed in the original PR #22.

I am working on a project that requires PID control and this crate is just perfect for my use case, so I forked it myself to include the changes and i'm using this fork as a dependency. If you think you can merge this into master please let me know.

@ghost ghost changed the title Add feature: Allow to manually set the integral term. Add features: A combination of other PRs + Making the API more flexible. May 14, 2024
@ghost
Copy link
Author

ghost commented May 14, 2024

Hello there. My idea with this PR at first was to just make a very small change in the hopes it would be quickly merged into master, but it turns out that my application required an API a bit more flexible on top of that.

So I decided to include more changes to my fork in order to provide the API I needed in my project. These changes also include some of the ones already made in PR #22 and #23.

Unfortunately this would be a breaking change for other applications already using your package, so it will certainly require a major release if it ever gets merged into master.

If you think these changes are interesting for your project but you still want to improve something I will be glad to contribute.

@braincore
Copy link
Owner

Hey @un3481! Thanks for the PR. I'm hoping I get some spare time to reacquaint myself with the code base and your changes. Assuming we get through all that, are you interested in becoming a maintainer?

Also, what's your use case for a pid?

@ghost
Copy link
Author

ghost commented May 25, 2024

Hi @braincore, just saw your reply now. I had to make even more changes as you can see in the lots of commits I made, and some of them were a bit of going back and forth with ideas that didint work, so sorry if that seems like a mess.

Well, my use case is the following: I am working on a project that aims to control the temperature of a specific kind of furnace using PID control while also being an embedded system with no-maintenance.

Initially we tried doing this with microcontrollers, and we ended up with a prototype using arduino and c++. But that quickly turned out to not be sufficient for our use case so we are now migrating to a raspberry pi prototype with the program itself written in rust.

In arduino I had simple PID libraries that worked well on a single thread scenario. But now I am doing it muti-threaded so I need the PID struct to have a static lifetime and access it through mutexes.

Other libraries seemed a little confusing or not appropriate for my use case, but your library is simple and works fine so I decided to use it, but I had to make some changes.

I am currently using this fork as a dependency, so it should compile, but I dont know if tests will pass and I could certainly improve some things.

If you eventually consider this PR good enough for merging I would be glad to be a contributor or even a maintainer. But there's no need to rush, please take your time to review the changes, and correct me if you find anything wrong.

@ghost ghost closed this by deleting the head repository Sep 17, 2024
This pull request was closed.
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.

1 participant