Air Conditioning Control - Feature Implementation

[Afroboltski]

AC control feature added, better than the existing idle-up feature (which can still be used for other things, e.g. electrical load detection).

Air conditioning is locked out with:
-Coolant temp above limit
-RPM above high limit
-RPM below low limit
-TPS above a high limit, with a re-engagement delay to prevent cutting in on gear changes

So the A/C automatically cuts out when driving hard :)

There is also the option of automatically starting the cooling fan any time the A/C compressor is running.

In addition, the idle duty cycle can be stepped up before the compressor engages (using the "A/C Compressor On Delay", which delays the A/C request signal from engaging the compressor, but does not delay the idle step or cooling fan activation).

The extra configuration data for A/C control is tacked into the unused bytes of page 13 - the programmable outputs page, so no additional pages were created.

Log entry size increased from 121 bytes to 122 bytes to allow for 1 byte of air conditioning data (bitfield). The bits are:

#define BIT_AIRCON_REQUEST 0 //Indicates whether the A/C button is pressed
#define BIT_AIRCON_COMPRESSOR 1 //Indicates whether the A/C compressor is running
#define BIT_AIRCON_LOCKOUT 2 //Indicates the A/C is locked out either due to the RPM being too high/low, or high coolant temp.
#define BIT_AIRCON_TPS_LOCKOUT 3 //Indicates the A/C is locked out due to high TPS, or the post-high-TPS "stand-down" lockout period
#define BIT_AIRCON_TURNING_ON 4 //Indicates the A/C request is on (i.e. A/C button pressed), the lockouts are off, however the start delay has not yet elapsed. This gives the idle up time to kick in before the compressor.
and bits 5-7 are unused.

Additional minor fixes in this pull request (sorry, I didn't think it was worth splitting it up):

-Air conditioning idle step up now works correctly with closed loop PWM, open loop PWM, and closed+open loop PWM. Untested with stepper motor, but no reason it shouldn't work. This was implemented incorrectly with the idle-up feature, and appeared only to work with open loop. The way the "closed loop duty cycle step" works is by artificially incrementing the feed-forward term (for open+closed loop), and by creating a "pseudo feed-forward" term inline for normal closed loop. (Side note - there appears to be nothing currently that cuts closed loop PWM idle control if you step on the throttle?)

-Fixed issue number #655 - aux. pins now initialise correctly

I appreciate any feedback and/or criticism! The testing I've done is fairly minimal as I don't have a running test car at the moment, however I've done sufficient testing to ensure the basic functionality works, and the configuration parameters are all written and used in the software correctly.

[kovama1]

This is so cool! I have AC in my car with speeduino and it gives me headache to constantly must think on it when i want overtake or do silly stuff on revlimitter :D

[HWright9]

Hey Great work, I had a look and have a couple of comments and suggestions:

1. I think that the AC delay on function should happen from any of the lockout reasons (RPM, coolant etc), Because you don't want it cycling on gear changes for example and just generally it will make sure you don't cycle the relay to the A/C too fast, especially since there is basically zero hysteresis on RPM etc.

In fact I would suggest a general "delay from off" state where if turned off for any reason, it won't request the compressor back on within XX sec (thinking 2-5sec is a reasonable range). This covers all sorts of intermitent request signals / bad wiring etc.

2. I don't think you have to keep checking "if(pinAirConComp != 0)" though the code, just have another state "AC DISABLED" or something that goes around the whole function to inhibit operation if the A/C is not enabled or setup correctly. - have you thought about coding this as a state machine? i.e. if (airConState == AIRCON_TURNING_ON) { Do State, then if conditions XX transition to new state}

3. There are already variables in the code that work like "engineRunSeconds" can you use any of them? Also if you are adding new variables that are only used for the A/C function perhaps have a naming convention like airConXXXXXXX so its easy to identify this variable is part of A/C system. Also is engineRunSeconds really seconds? The code is called at 4Hz.

4. Formatting, Understand this is just my opinion here, I am sure your code works.
   Can you use some brackets around your conditional statements, it makes it clearer. Also for myself once I get past 2 conditionals or where it's complicated I like to put them on new lines. i.e.
   if ((currentStatus.coolant > offTemp) || (currentStatus.RPMdiv100 < configPage13.airConMinRPM || currentStatus.RPMdiv100 > configPage13.airConMaxRPM))

becomes

if ( (currentStatus.coolant > offTemp) || 
     (currentStatus.RPMdiv100 < configPage13.airConMinRPM) || 
     (currentStatus.RPMdiv100 > configPage13.airConMaxRPM) )

one line updates and comparisons, sometimes make it tricky to follow

if(++acStartDelay >= configPage13.airConCompOnDelay)

could be

acStartDelay++;
if(acStartDelay >= configPage13.airConCompOnDelay)

It's minor but does make it clear as to the order of operations.

[Afroboltski]

Thanks for the feedback, HWright9!

1. There is the normal "AC On Delay" which happens at the "I/O level" already, whether the lockout is TPS, RPM, or coolant temperature. I've set that default value at 0.5s as it's primarily to allow idle up (if present) to preemptively happen before the compressor loading. This also has the effect of filtering due to dodgy connections/etc. The TPS lockout is specifically for high TPS conditions, during gear changes as you so mention. I guess I could add a couple more Yes/No options, e.g. the user could be able to choose if they want the extended "TPS lockout delay" to also be applied to high RPM conditions? Regarding coolant temp, I don't think this would ever change fast enough (hopefully) for it to be a problem.

2. Good call, I'll do that. My main concern is an issue I see someone has bought up before. If pinAirCon is zero, I don't want the A/C software to call digitalWrite (or the simplified, non-interrupt safe? *aircon_comp_pin_port |= (aircon_comp_pin_mask)), especially seeing as pin 0 is a valid pin on the Arduino mega! However pin 0 seems to be a speeduino convention representing "unused". I believe it's actually one of the Serial I/O pins (RX).

3. Good point, my engineRunSeconds is actually engineRun250ms, and is already scaled in the TS ini file as such. The problem with the existing variable currentStatus.runSecs is it doesn't appear to take into account if the engine is stalled and restarted without resetting the Arduino. So it represents "seconds since engine was initially cranked over and started" as opposed to "engine running seconds". In the case that the A/C RPM low limit was set to 0 and the car stalled, the A/C compressor would remain pulled in if this variable was used. Conversely, I could use the runSecsX10 variable defined in globals.ino, which does reset if the engine stops, however it's a pet peeve of mine when variables are not checked for overflow, when an overflow would cause a glitch........ even if the overflow won't happen for 119,000 hours or 13 years! 😂 I guess the proper fix is to just use runSecX10 variable and sneak an overflow check in there...

4. Good call, will do in the next revision. Normally I subconsciously forego multi-lining 'if' statements if the variables/comments/methods are named well enough, e.g if it's turning on a variable called "AIRCON_LOCKOUT" in pretty sure I know what's happening, but I overlooked the fact that other people have to look at it here... sorry, my first time contributing to an open source project :)

EDIT:
I see some irony relating to point number 3:
if(++acStartDelay >= configPage13.airConCompOnDelay) { AIRCON_ON(); }

should be

if(acStartDelay >= configPage13.airConCompOnDelay) { AIRCON_ON(); } else { acStartDelay++; }

otherwise acStartDelay will overflow...

[Afroboltski]

There we go, there's now another configurable compressor re-engagement delay for the RPM high/low lockout 😎

All done, I'm not planning on making any more changes.

Maybe a feature to add in the future could be on-board compressor cycling based on a refrigerant pressure sensor, or A/C compressor discharge temperature (as someone mentioned on Slack). However for the target application here, the car should have a pressure/temperature switch that cuts the A/C request signal to the ECU (external to the engine management system).

[shiznit304]

Great stuff! I was wanting something like this early on in my speeduino build but was able to get all of it sorted with just programmable outputs. Check out my PR for idle up rpm adder, this helped tremendously keep my RPMs stable when the a/c cycles on and off by itself: #586

[Corey-Harding]

I have finally got my ac and idle up working properly using programmable outputs, my ac condensor fan is one of those outputs and ac compressor relay the other, would it be reasonable to add ac condensor fan control as an option that people who dont need it seperate can just disable it, but people who need it can choose output pin and delay?

Thanks for all your help so far and I am thinking I might start testing your pr soon.

[Afroboltski]

I'll give it a shot, although it looks like noiymime's latest commit for some SD card logging stuff has kicked me out of EEPROM page 13 in TS... I might have to shuffle some stuff around and/or make a new page.

[Afroboltski]

The dedicated A/C fan output has been added, along with moving my config data to page 9 to avoid noisymime's SD logging EEPROM data.

Some of the merging between the SD logging software and the extras I'd tacked onto my last commit got a bit messy, I'd appreciate a second pare of eyes over it ;)

[vanyasvl]

@Afroboltski where I can find TunerStudio ini for that new A/C config?

[Afroboltski]

@Afroboltski where I can find TunerStudio ini for that new A/C config?

It's not ready to be merged with the master branch yet, unfortunately.

You can freely download my forked code I believe (i.e. go to https://github.com/Afroboltski/speeduino) but it's based on an old version of the master branch, and not extensively tested.

I'll hopefully get it ready to be merged with the master branch before the next official FW release.

[Corey-Harding]

I tried to get @HazuTo25 's Extra Features Page 15 changes(was added for this PR) up to date with the current master although I haven't looked over the whole thing in detail yet but maybe it will help you two by posting it here. It compiles without errors but like I said I have not tested it or looked over it much at all.

Edit:

https://github.com/Corey-Harding/speeduino/tree/AC-Control-Clean-3

[FazxFi]

I tried to get @HazuTo25 's Extra Features Page 15 changes(was added for this PR) up to date with the current master although I haven't looked over the whole thing in detail yet but maybe it will help you two by posting it here. It compiles without errors but like I said I have not tested it or looked over it much at all.

About that Page 15 stuff, I haven't really tested it, so I don't really sure that it's gonna work without any issue.
But feel free to test it if it works ok for you.

My main concern/problem to address is here as this needs to be added back in
https://github.com/Corey-Harding/speeduino/blob/AC_Control_Clean/reference/speeduino.ini#L5119
Line 5119-5121 in speeduino.ini needs to be restored properly

;sd_filenum = scalar, U16, 123, "", 1, 0
;sd_error = scalar, U08, 125, "", 1, 0
;sd_phase = scalar, U08, 126, "", 1, 0

EDIT: I couldn't find any references to the above when looking through the code so we might be ok on that. Anyways someone please let me know if they end up looking this over. I may start making actual changes to this version of the PR in my branch soon and test it out myself but it may be a while before I get some time to do so properly.

What do you mean about that? Do you mean about the line is being commented or because I change the "bits".

I want to add an airconfan IS ON indicator to one of the unused bits and might clean up some of the language and comments but if everything is functional I can't think of anything else I would want from this PR.

I read this and I work on that yesterday, I have done that but haven't "push" it yet, because I am not sure it even work, and I want to fix other stuff too.

disclaimer: I don't really understand how to work with programing language, especially with C/C#/C++.
Don't expect my coding to work 100%. And I usually just copy other people's code and integrate it with my code and use it on my own at my own risk

But I hope I can help anything that I could ( :

[Corey-Harding]

Disregard the speeduino.ini comment I need to check again but think its a nonissue.

However check my update.ino comment above

Pretty sure all it needs is something like
configPage15.airConEnable = false;
But I need to look at it and see what it actually needs to be

This is a typo, should be 128?
https://github.com/Corey-Harding/speeduino/blob/6d45e9173bb4f31dc3f29085cfe2f933a80cdae7/speeduino/storage.cpp#L287

Glancing over I think there is a typo here also
https://github.com/Corey-Harding/speeduino/blob/6d45e9173bb4f31dc3f29085cfe2f933a80cdae7/speeduino/globals.h#L1461

I will look at it all in more detail eventually.

[FazxFi]

This is a typo, should be 128?
https://github.com/Corey-Harding/speeduino/blob/6d45e9173bb4f31dc3f29085cfe2f933a80cdae7/speeduino/storage.cpp#L287

Yes, It was a typo.

Glancing over I think there is a typo here also
https://github.com/Corey-Harding/speeduino/blob/6d45e9173bb4f31dc3f29085cfe2f933a80cdae7/speeduino/globals.h#L1461

This too.
It just a comment for a reminder so it doesn't really a problem, but tq for pointing it out.
I just remember why I put 192 there, page.ccp giving me an error so I tried different size, but later I found out the reason,
It's because in global.h file I put wrong bits number. : p

BELOW CHANGES NEED TO BE MADE TO THE AC CONTROL BRANCH I LINKED ABOVE BEFORE TESTING:

updates.ino
Need to update CURRENT_DATA_VERSION in updates.ino to 21

And add

if(readEEPROMVersion() == 20)
{
//comment
Pseudo configpage15 update sequence here

writeAllConfig();
storeEEPROMVersion(21);

}

storage.h
Also need to update line 7 of storage.h to reflect version 21

You using the development firmware right, but my branch still using the release version of the firmware just because,
so I will do it in current version.

I just gonna put it in
if(readEEPROMVersion() == 19) line

I don't know if this the right way to do it.
But you can just do it just do it like you show before.

Edit: Do you need to put only one or all of it?

[Afroboltski]

Are we trying to get the AC Control ready to merge for the next firmware release? 😃

Let me know what I can do to help.

[Corey-Harding]

Here is the branch that is "mergeable without conflicts" although it needs to be tested.

@HazuTo25 added everything to Page 15 and I brought it up to date with current master.

https://github.com/Corey-Harding/speeduino/tree/AC-Control-Clean-3

[FazxFi]

In https://github.com/Corey-Harding/speeduino/blob/AC_Control_Clean/reference/speeduino.ini#L1392
Unused15_13_128 = array, U08, 13, [115], "", 1, 0, 0, 255, 0
128 should be 127?
Unused15_13_127 = array, U08, 13, [115], "", 1, 0, 0, 255, 0
The 192 length pages only went to 191
https://github.com/Corey-Harding/speeduino/blob/AC_Control_Clean/reference/speeduino.ini#L1047

yes, it should be 127, I just realize that. (either way it just on a name to show how much left in what page size?)
actually I don't know, someone know

Also looks like this line needs to be added to ini and/or me look over and merge the indicator commit by @HazuTo25
FazxFi@9335bdb#diff-3a79112606ac8fec5bc323aab7f9332785973542dc4ef9f3675e3ef8afd5e71bR5070

meaning?

Are we trying to get the AC Control ready to merge for the next firmware release? 😃

Let me know what I can do to help.

maybe you can update some of your code on @Corey-Harding or my repo.
ae idle.ino ?

like I said before, I am not really familiar with this programing language.

[FazxFi]

Question

f6cb290

Do this code work for ac fan indicator?
#define AIRCON_FAN_ON() {((((configPage15.airConFanPol&1)==1)) ? AIRCON_FAN_PIN_LOW() : AIRCON_FAN_PIN_HIGH()); BIT_SET(currentStatus.airConStatus, BIT_AIRCON_FAN);}
#define AIRCON_FAN_OFF() {((((configPage15.airConFanPol&1)==1)) ? AIRCON_FAN_PIN_HIGH() : AIRCON_FAN_PIN_LOW()); BIT_CLEAR(currentStatus.airConStatus, BIT_AIRCON_FAN);}

I follow this line in auxiliary.h
#define AIRCON_ON() {((((configPage15.airConCompPol&1)==1)) ? AIRCON_PIN_LOW() : AIRCON_PIN_HIGH()); BIT_SET(currentStatus.airConStatus, BIT_AIRCON_COMPRESSOR);}
#define AIRCON_OFF() {((((configPage15.airConCompPol&1)==1)) ? AIRCON_PIN_HIGH() : AIRCON_PIN_LOW()); BIT_CLEAR(currentStatus.airConStatus, BIT_AIRCON_COMPRESSOR);}

[Corey-Harding]

I merged @HazuTo25 's fan indicator changes but also caught this, the coolant lockout bit was overlooked right?

Corey-Harding@d17c1cb

I think its close to the point that I will try to test although it may take a couple weeks to get around to it. It is still too cold here for AC and I like to test during my normal daily driving conditions.

https://github.com/Corey-Harding/speeduino/tree/AC-Control-Clean-3

[Afroboltski]

I merged @HazuTo25 's fan indicator changes but also caught this, the coolant lockout bit was overlooked right?

Corey-Harding@d17c1cb

I think its close to the point that I will try to test although it may take a couple weeks to get around to it. It is still too cold here for AC and I like to test during my normal daily driving conditions.

https://github.com/Corey-Harding/speeduino/tree/AC-Control-Clean-2

Nice catch! My initial implementation just had a single "lockout" bit, then I separated the three lockouts out for reasons that I can't remember.

So to confirm, you need nothing else from me in order to get this ready for the next firmware release? I'm running with the assumption here that you're on the development team 😛

[Afroboltski]

Adding yet another commit - just a minor change. Adding the 3 inline static function prototypes to auxiliaries.h, as per the Style Guide.

[floods57]

See last 2 commits, the bug is fixed. It was a problem with the page 15 stuff, the bytes were misaligned.
Should be all good to go now!

That working, all the function is working great now, i just notice a warning in TS here the warning i get : Warning: Failed to set value for unusedClusterBits .msq value is not valid for current configuration: No valid options found for Bit EcuParameter:unusedClusterBits equal to the proposed 15
Look like that not causing any issue, but not sure that in you code, i have the same warning with the official lasted version (not the released one). Thanks

Thats unrelated from another PR. All the values are set to invalid. See #829

Ok, thanks, i will check that.

[kennyevo]

Wow, I was just thinking about using my speeduino in my 1zz turbo car, but didn't want to lose the AC, looks like there is excellent work done here, can't wait to try it!

[Afroboltski]

Wow, I was just thinking about using my speeduino in my 1zz turbo car, but didn't want to lose the AC, looks like there is excellent work done here, can't wait to try it!

Cheers, I appreciate the feedback!

[iltHeo84]

Hello, and thanks for the great work!
Now that A/C Control is merged into the master repository, I would like to use it as a second Idle Up pin (my car has no A/C).
Would the code work with the Output Pin set to 0? Or should I pick an unused pin?
Thanks

[mbbrinkman]

I'm having some issues configuring this. If I have an idle-up input pin set to normal, I assume that using that same pin as the AC request input is fine. But the idle-up and RPM adding don't seem to be engaging.

Idle up works ordinarily just fine.

[Afroboltski]

Hello, and thanks for the great work! Now that A/C Control is merged into the master repository, I would like to use it as a second Idle Up pin (my car has no A/C). Would the code work with the Output Pin set to 0? Or should I pick an unused pin? Thanks

I just had a look - the way I wrote it, the air compressor pin has to be non-zero for the whole function to work - so you'd have to set it to an unused pin.

I'm having some issues configuring this. If I have an idle-up input pin set to normal, I assume that using that same pin as the AC request input is fine. But the idle-up and RPM adding don't seem to be engaging.

Idle up works ordinarily just fine.

Hard to say without more info unfortunately! I suppose if you accidentally set the "Air Compressor Output" pin to be the same as the "Idle Up Input" pin, both functions may not work (Idle up due to the pinIsOutput function and air con because you didn't set the "AC Request" pin.

[mbbrinkman]

I don't have an output pin used as my circuit to engage the compressor is just a simple relay triggered by the physical button in my dash. My ECU is only used for idle up via IACV which already has a set pin it can use.

Here's how I have it.

If I enable Air conditioning control or leave it on while keeping idle up on (in the idle up settings window), I still have normal idle up behavior as it is set in the separate idle-up window, but do not appear to have the rpm adder working or the idle up that is set in the AC control working.

If I disable idle up in that idle up settings window and turn on AC control, it does not appear to idle up at all however or add RPMs to the target. It seems to behave as if the AC control does nothing.

Maybe this should be a separate feature request but the idle up in AC control and the separate idle up window explicitly can conflict (ie can set one to 10 and other to 20). I am not sure which takes priority or if one should be removed to stop a conflict as there might be other reasons of having idle up separate from AC control.