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

Report 40°C on virtual TMC heater channel for any driver being active #180

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

wilriker
Copy link
Contributor

This one closes https://forum.duet3d.com/topic/5536/status-based-fan-mode - if I did not miss anything where else it would be required to check for an non-disabled driver. ;-)

@wilriker wilriker force-pushed the pseudo-temp-for-active-driver branch from ad87247 to 65a43c7 Compare June 19, 2018 11:06
@wilriker
Copy link
Contributor Author

I have amended this commit as I forgot to add the const modifier to the new method that checks if any driver is currently active/idle.

@wilriker wilriker force-pushed the pseudo-temp-for-active-driver branch from 65a43c7 to 9967e84 Compare July 24, 2018 10:15
Copy link
Collaborator

@dc42 dc42 left a comment

Choose a reason for hiding this comment

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

I cannot accept this PR as it stands because it doesn't distinguish between drivers that are on the Duet, drivers that are on a DueX2 or DueX5, and external drivers. Each virtual heater channel should show 50C only when relevant drivers are active.

@wilriker
Copy link
Contributor Author

wilriker commented Aug 7, 2018

I will fix that on Thursday. Thanks for spotting.

@wilriker
Copy link
Contributor Author

wilriker commented Aug 9, 2018

While adjusting this I found something strange. There are up to 10 drivers when using Duet+DueX5. But there is constexpr size_t DRIVES = 9; in Pins_Duet.h and this limits it to only 9 drivers max. Why is that? How is the last driver on DueX5 used at all?

Everything I found related to drivers has only a length of 9 including the enable step and direction pins.

What am I missing? I was missing that there are several definitions of this constant. 🤦‍♂️ Out of some reason they are hidden in Project Explorer view of eclipse.

@wilriker wilriker force-pushed the pseudo-temp-for-active-driver branch from 9967e84 to 692b48c Compare August 9, 2018 10:57
@wilriker
Copy link
Contributor Author

wilriker commented Aug 9, 2018

Fixed. This will now check a maximum of 5 drivers per board. This means it will never check drivers 10 and 11 unless there will be a third board requested.

@wilriker wilriker force-pushed the pseudo-temp-for-active-driver branch from 692b48c to 42e4ec9 Compare August 13, 2018 11:56
@wilriker
Copy link
Contributor Author

wilriker commented Jan 4, 2019

Any update on accepting this one?

@dc42
Copy link
Collaborator

dc42 commented Jan 4, 2019

Thanks for the reminder. Why did you choose 50C? The complication I can see is that if you have a fan set to run thermostatically on both CPU temperature and driver temperature, you can only choose one temperature for both. So the value we choose to report for drivers that are active matters.

@wilriker
Copy link
Contributor Author

wilriker commented Jan 4, 2019

50C was just the logical gap between the existing

  • 0C: everything OK
  • 100C: warning
  • 150C: critical

Do you have any recommendation for a better value? My only idea right now is to set it at the value when it would be recommended for the CPU to be cooled. So e.g. if it is recommended to cool the CPU once it reaches 42C then set this value also to 42C. This way if the drivers are off but the CPU reaches this temperature out of whatever reason (extensive simulations, pointing a heat-gun at it?! ;-) ) cooling would still start as necessary.

@wilriker
Copy link
Contributor Author

wilriker commented Jan 11, 2019

I thought about the temperature a little and I think 35C or 40C would probably be a better temperature in regards to thermostatically controlled fans that also monitors CPU. 35C might be a bit over-cautious but I think 40C would be good.

What do you think?

@wilriker wilriker changed the base branch from v2-dev to dev January 14, 2019 19:42
@wilriker wilriker force-pushed the pseudo-temp-for-active-driver branch from 42e4ec9 to 32cd699 Compare February 1, 2019 14:36
@wilriker wilriker changed the title Report 50°C on virtual TMC heater channel for any driver being active Report 40°C on virtual TMC heater channel for any driver being active Feb 1, 2019
@wilriker
Copy link
Contributor Author

wilriker commented Feb 1, 2019

I have updated this PR to the latest commit on dev branch since there were some incompatible changes sometime in between. I also hid the method checking for active drivers behind

#if HAS_SMART_DRIVERS
...
#endif

and let it only check up until MaxSmartDrivers instead of MaxTotalDrivers. This can be easily changed back in case this method needs to be used somehow on Duet 0.6/0.85.

Also I lowered the returned temperature to 40°C.

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