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: https://github.com/noisymime/speeduino/pull/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 https://github.com/HazuTo25/speeduino/commit/9335bdbedce6a6066ec9f9e4e98adbefba9f3e32#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?

https://github.com/Corey-Harding/speeduino/commit/d17c1cbb18c02aae57bd06d75d441a773c9c21be

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 😛

[Corey-Harding]

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 😛

Me? Development Team? I'm just a facilitator on this one. @HazuTo25 is your dev team, they are just humble. I didn't do anything significant, just tried bringing you two together. If you wanted to it would be nice if you looked it over and see if I missed anything and that I didn't mess up any of @HazuTo25 's page15 changes. Feel free to leave notes or PR. I have done zero testing so far and have very casually been making changes while multitasking so its very possible that there are some issues I overlooked. I honestly kept hoping you would just pick it up. :). (Edit: If you mean beyond the scope of this PR definitely not)

I was excited about your PR and really wanted to give your PR a good shot at getting merged and wasn't sure if you were still working on it. Sorry to have kind of taken over the conversation thread.

I think its at the point that I need to test it and if its working good maybe clean up the comments and make sure everything makes sense and then figure out if you wanted to copy it over to your branch on this PR or if I submit it and put your and @HazuTo25 's name on it. Your in charge of that decision. Its your work. I'm also good scrapping it if you already have a better solution or if you didn't like this direction. @HazuTo25 said their branch is pretty much personal use, mine is mainly that way but I also like keeping an easy upgrade path for myself so I try not to deviate too far from master.

Anyways thanks @Afroboltski and @HazuTo25 for your hard work on this. I hope it ends up going somewhere.

[Corey-Harding]

And... It does not work :-/

AC request never triggered. Its below freezing here today so it will be a while before I try to mess with this again.

[FazxFi]

And... It does not work :-/

AC request never triggered. Its below freezing here today so it will be a while before I try to mess with this again.

That what I thought, : ( I hope I still had that firmware without that page 15 but it still 202202 version, to test if it work and make sure what is causing it.

I think I had the backup somewhere

edit: I found it. It is actually the early 202204-dev (around February) question @afroboltski

Line 39 #define AIRCON_FAN_OFF() (((configPage9.airConFanPol&1)==1) ? AIRCON_FAN_PIN_HIGH() : AIRCON_FAN_PIN_LOW());

Is this right?

compare to this line Line 38 #define AIRCON_FAN_ON() ((((configPage9.airConFanPol&1)==1)) ? AIRCON_FAN_PIN_LOW() : AIRCON_FAN_PIN_HIGH()); this is from origarnal code from this PR

• its missing "( )" I want to make sure

[FazxFi]

and by the way

And... It does not work :-/

AC request never triggered. Its below freezing here today so it will be a while before I try to mess with this again.

you can try get rid of this line in the ini file at line 5070? airConStatus = scalar, U08, 122, "bits", 1.000, 0.000

I can't try any of the code because my ecu is kinda fry.

[Corey-Harding]

I need to look at this in more detail sometime. All I did was get rid of the merge conflicts.

[Afroboltski]

And... It does not work :-/

AC request never triggered. Its below freezing here today so it will be a while before I try to mess with this again.

Ahhh ha.

You seem to missing the following lines from auxiliaries.h:

volatile PORT_TYPE *aircon_comp_pin_port; volatile PINMASK_TYPE aircon_comp_pin_mask; volatile PORT_TYPE *aircon_fan_pin_port; volatile PINMASK_TYPE aircon_fan_pin_mask; volatile PORT_TYPE *aircon_req_pin_port; volatile PINMASK_TYPE aircon_req_pin_mask;

I dunno if that's exhaustive or not, just what I found when I went through your AC-Control-Clean-2 branch.

[Corey-Harding]

@Afroboltski Those lines were there, you must have been looking at some commit history instead of the whole file, I need to make an AC Clean 3 branch where all the changes are in a single commit.

https://github.com/Corey-Harding/speeduino/blob/AC-Control-Clean-2/speeduino/auxiliaries.h#L66

[FazxFi]

@Afroboltski @Corey-Harding how about this line in global.h byte airConEnable : 1; byte airConCompPol : 1; byte airConReqPol : 1; byte airConTurnsFanOn : 1; byte airConFanEnabled : 1; byte airConFanPol : 3;

//Bytes 1-12 - Air conditioning analog points byte airConCompPin; byte airConReqPin;

shouldn't it be

byte airConEnable : 1; byte airConCompPol : 1; byte airConReqPol : 1; byte airConTurnsFanOn : 1; byte airConFanEnabled : 1; byte airConFanPol : 1;

//Bytes 1-12 - Air conditioning analog points byte airConCompPin : 6; byte airConReqPin : 6;

The number 6 is either 5 or 6,I am not sure

[Corey-Harding]

I didn't make any changes or look into it yet but here is all of the changes in a single commit that might be easier to review.

https://github.com/Corey-Harding/speeduino/commit/b988540e8296285d5da0b17d002f84ac0b009a9d

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

Edit: I'm not confident that I am going to retain my AC at this point so I may not get back around to looking into this further, plus its still cold here. Also I got to thinking that I did not actually test my AC switch with a multimeter to make sure it was actually working as expected so this PR could actually be good.

[Afroboltski]

I didn't make any changes or look into it yet but here is all of the changes in a single commit that might be easier to review.

Corey-Harding@b988540

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

Edit: I'm not confident that I am going to retain my AC at this point so I may not get back around to looking into this further, plus its still cold here. Also I got to thinking that I did not actually test my AC switch with a multimeter to make sure it was actually working as expected so this PR could actually be good.

@Corey-Harding I just scoured your AC-Control-Clean-3 branch, I couldn't see anything wrong with the naked eye. The pages size array, bit/byte offsets all seem to add up, etc. By the way, nice work with the extra page 15, I didn't think of doing that at first, nor did I think a quick A/C feature was enough work to be necessary. That's one way to avoid collisions with other dev work! Had I known there would be so much other concurrent work kicking me out of the spare bits in existing pages of which I tried to take advantage, I would have considered this path with more thought.

I've got a lone Arduino Mega I can test on again if you're not gonna get around to it any time soon? Unfortunately I don't have a speeduino-powered vehicle at the moment, so I end up resorting to fudging lines out of the code to make it think the engine is running, coolant is a reasonable value, etc... not the best way to test for sure. An AVR simulation tool would be helpful if I had the time to set one up! I did test my initial PR successfully; something might have gone wrong in the move to config page 15 I reckon.

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);}

@HazuTo25 there is supposed to be both AIRCON_ON() and AIRCON_FAN_ON().

AIRCON_FAN_ON() is used in the rare case that you have a stand-alone A/C fan, maybe side-by-side with your radiator cooling fan. This turns on straight away with the A/C demand. If you just have a normal radiator fan that also turns on for A/C, you can see this is triggered in the fanControl() method, if you have airConTurnsFanOn set to true.

AIRCON_ON() engages the compressor itself. This one is also called after the compressor delay, which was intended to give the idle control a split second to idle up BEFORE the A/C compressor load kicks in. This is how Nissan, and other commercial aftermarket ECU manufacturers do it.

By the way, even if airConTurnsFanOn is set to true, it currently doesn't work if you are using PWM fan. Maybe some kind of feed-forward term similar to the idle control can be added later.

[Corey-Harding]

I didn't notice the offending issue either. It might work, I could have had an issue with my physical ac switch or that input. I was still trying on A8 and its never been that straight forward for me.

Anyways. I am deleting AC. So please test it. I won't really be messing with this much anymore. Btw there are new merge conflicts. I have not reviewed them though.

[FazxFi]

https://github.com/HazuTo25/speeduino/tree/Page-15 @Corey-Harding Here is the working Page 15 stuff incase you want to use it (at least to some extent of my testing) -I can't really test for the ac, but I think it just cannot read the pin input for some reason, or it is just my Arduino problem

[Corey-Harding]

Here is my branch that now has merge conflicts with current master but it didnt work when I tested it with A8 in its current state. I also never thoroughly reviewed it.

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

Want me to resolve them or should I delete this branch? Looks like your starting over? Again I am deleting my AC so I won't really be working on this anymore.

[Afroboltski]

Here is my branch that now has merge conflicts with current master but it didnt work when I tested it with A8 in its current state. I also never thoroughly reviewed it.

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

Want me to resolve them or should I delete this branch? Looks like your starting over? Again I am deleting my AC so I won't really be working on this anymore.

Cheers @Corey-Harding! Yeah I reset my branch to master last night, and I've now merged all the changes from your AC-Control-Clean-3 commit. Just finalising the testing now - it seems to work exactly as expected. I'll commit shortly.

I did find the pullup wasn't working for pin 27 properly (but worked on different pins), but then I discovered the test tune I have had a programmable output assigned to pin 27, which supposedly calls pinMode(..., OUTPUT); BEFORE the A/C setup. I guess even calling pinMode(..., INPUT_PULLUP); afterwards, the digitalWrite(...) statements still cause it not to work. I guess the I/O protection should be extended at some point (not related to the A/C stuff anyway) to cover this.

@HazuTo25 cheers, I've used the page 15 stuff, as well as the update logic just like you've pushed in your branch.

Quick question, can I put my changes into EEPROM version 20, assuming my PR will be merged into the 202204 release (if there is such a release coming up)? That way we avoid 2 EEPROM version increments in the same release I suppose.

[Afroboltski]

@Corey-Harding and @HazuTo25, I've tested the following on this branch, which is now fully up-to-date with the master branch:

• A/C request read correctly.
• A/C compressor output turns on correctly, after the A/C compressor engage delay.
• High TPS locks out the A/C compressor for the specified delay period.
• High ECT locks out the A/C compressor for the specified delay period.
• High/Low RPM lock out the A/C compressor for the specified delay period.
• A/C idle-up engages correctly, prior to the A/C compressor engage delay. Tested for OL PWM, CL PWM, and OL+CL PWM. Stepper motor idle-up not tested, but compiles fine and is really just copied from the idle-up adder PR.
• Engine fan triggered correctly (if setting activated), prior to the A/C compressor engage delay.
• Stand-alone A/C fan triggered correctly (if setting activated), prior to the A/C compressor engage delay.

Couple of notes:

• AC request "normal" polarity (with internal pull-up) means the AC request is on when the request pin is driven to GND. "Inverted" needs +5V for the request to be on, and an external pull-down would be needed (not tested).
• The existing idle-up adder was not implemented correctly for some of the idle algorithms - the duty cycle was being added to currentStatus.idleLoad after the PWM target was already calculated, meaning the duty cycle would display in TS correctly, but the car wouldn't actually idle up. I did a quick fix in this PR (only partially tested).
• The closed-loop idle-up for A/C was implemented in a sort of "pseudo feedforward" kind of way. The idle target does not change, you just get a duty cycle step to "catch" the extra load of the compressor engaging. By the way, the closed loop idle doesn't appear to lock out correctly when the TPS is high, I guess it just sits at the minimum limit. In my opinion, this idle algorithm should be removed in favour of OL+CL, because you could always just zero-out the OL terms?

Let's get this into the next FW release!

[FazxFi]

Ur welcome, btw the ac bits, I think you can put the fan pin with the other like this.

struct config15 {
  //Byte 0 - 3 Air conditioning setup
  byte airConEnable     :1;
  byte airConReqPol     :1;
  byte airConReqPin     :6; //8
  byte airConCompPol    :1;
  byte airConCompPin    :6; 
  byte airConTurnsFanOn :1; //8
  byte airConFanEnabled :1;
  byte airConFanPol     :1;
  byte airConFanPin     :6; //8

  //Bytes 4-11 - Air conditioning parameter 
  byte airConTPSCut;
  byte airConMinRPMdiv100;
  byte airConMaxRPMdiv100;
  byte airConClTempCut;
  byte airConIdleSteps;
  byte airConTPSCutTime;
  byte airConCompOnDelay;
  byte airConAfterStartDelay;
  byte airConRPMCutTime;

  //Bytes 12-128
  byte Unused15_12_127[116];

and it reflect in another file as well.

Quick question, can I put my changes into EEPROM version 20, assuming my PR will be merged into the 202204 release (if there is such a release coming up)? That way we avoid 2 EEPROM version increments in the same release I suppose.

If there are think just put it in version 19, unless you want change the "current_data_version" to 21, that's just my thinking.

Btw the idle.ino had some update, it had some update on something you change on your code, mind to check that out?

[Afroboltski]

struct config15 {
  //Byte 0 - 3 Air conditioning setup
  byte airConEnable     :1;
  byte airConReqPol     :1;
  byte airConReqPin     :6; //8
  byte airConCompPol    :1;
  byte airConCompPin    :6; 
  byte airConTurnsFanOn :1; //8
  byte airConFanEnabled :1;
  byte airConFanPol     :1;
  byte airConFanPin     :6; //8

  //Bytes 4-11 - Air conditioning parameter 
  byte airConTPSCut;
  byte airConMinRPMdiv100;
  byte airConMaxRPMdiv100;
  byte airConClTempCut;
  byte airConIdleSteps;
  byte airConTPSCutTime;
  byte airConCompOnDelay;
  byte airConAfterStartDelay;
  byte airConRPMCutTime;

  //Bytes 12-128
  byte Unused15_12_127[116];

and it reflect in another file as well.

Is this necessary given you only save 1 byte doing it that way? I prefer all analogs to have their own byte, and then 1 byte per 8 packed digital values.

Quick question, can I put my changes into EEPROM version 20, assuming my PR will be merged into the 202204 release (if there is such a release coming up)? That way we avoid 2 EEPROM version increments in the same release I suppose.

If there are think just put it in version 19, unless you want change the "current_data_version" to 21, that's just my thinking.

Cheers, yeah that's what I've done. It's in the transition from 19 -> 20, so the constant at the top of the file is #define CURRENT_DATA_VERSION 20, it's in the code that starts with if(readEEPROMVersion()==19)

Btw the idle.ino had some update, it had some update on something you change on your code, mind to check that out?

Yes, as per my comment below, it a). fixes the problem with the existing idle-up adder (#586), and b). idles up when A/C engages. Maybe @shiznit304 can comment on the idle-up adder functionality a bit more 😀. Or perhaps we should revert these changes and fix them in another PR? Or maybe in shiznit304's branch, the problem is already fixed? I'm just going off the main, master branch here.

a).

• The existing idle-up adder was not implemented correctly for some of the idle algorithms - the duty cycle was being added to currentStatus.idleLoad after the PWM target was already calculated, meaning the duty cycle would display in TS correctly, but the car wouldn't actually idle up. I did a quick fix in this PR (only partially tested).

b).

• The closed-loop idle-up for A/C was implemented in a sort of "pseudo feedforward" kind of way. The idle target does not change, you just get a duty cycle step to "catch" the extra load of the compressor engaging. By the way, the closed loop idle doesn't appear to lock out correctly when the TPS is high, I guess it just sits at the minimum limit. In my opinion, this idle algorithm should be removed in favour of OL+CL, because you could always just zero-out the OL terms?

By the way, I did not test point a). with an oscilloscope, but going through the code you can easily spot the mistake 😉.

[floods57]

Hi, I looking to try your AC control, but i get a error when verify under Arduino IDE, here the error i get : In file included from globals.h:31:0, from storage.cpp:10: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from page_crc.cpp:1: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from pages.cpp:2: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from comms.cpp:9: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from speeduino.ino:24: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from comms_legacy.cpp:9: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ exit status 1 static assertion failed: Number of header table titles must match number of log fields Did i do anything wrong ? the official firmware don't cause me this issue and i can upload it no problem.

Thanks a lot

EDIT : i found the error, in logger.h line 248 remove /* (must only be after header_89,\ not before)

[Afroboltski]

Hi, I looking to try your AC control, but i get a error when verify under Arduino IDE, here the error i get : In file included from globals.h:31:0, from storage.cpp:10: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from page_crc.cpp:1: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from pages.cpp:2: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from comms.cpp:9: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from speeduino.ino:24: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from comms_legacy.cpp:9: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ exit status 1 static assertion failed: Number of header table titles must match number of log fields Did i do anything wrong ? the official firmware don't cause me this issue and i can upload it no problem.

Thanks a lot

EDIT : i found the error, in logger.h line 248 remove /* (must only be after header_89,\ not before)

Cheers, yeah I just noticed when I merged the master branch commit 73badbce8ca171faa8c58575947917829adfc1ba (after I did my commit, 91e6e33ece5882a03e449738cab6e997ee9e66ec), the line with the extra /* was added back in.

[shiznit304]

The change control in this pr is a huge mess lol

On Sun, May 1, 2022 at 6:05 PM Afroboltski @.***> wrote:

Hi, I looking to try your AC control, but i get a error when verify under Arduino IDE, here the error i get : In file included from globals.h:31:0, from storage.cpp:10: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from page_crc.cpp:1: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from pages.cpp:2: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from comms.cpp:9: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from speeduino.ino:24: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ In file included from globals.h:31:0, from comms_legacy.cpp:9: logger.h:286:1: error: static assertion failed: Number of header table titles must match number of log fields static_assert(sizeof(header_table) == (sizeof(char*) * SD_LOG_NUM_FIELDS), "Number of header table titles must match number of log fields"); ^~~~~~~~~~~~~ exit status 1 static assertion failed: Number of header table titles must match number of log fields Did i do anything wrong ? the official firmware don't cause me this issue and i can upload it no problem.

Thanks a lot

EDIT : i found the error, in logger.h line 248 remove /* (must only be after header_89,\ not before)

Cheers, yeah I just noticed when I merged the master branch commit 73badbc https://github.com/noisymime/speeduino/commit/73badbce8ca171faa8c58575947917829adfc1ba (after I did my commit, 91e6e33 https://github.com/noisymime/speeduino/commit/91e6e33ece5882a03e449738cab6e997ee9e66ec), the line with the extra /* was added back in.

— Reply to this email directly, view it on GitHub https://github.com/noisymime/speeduino/pull/665#issuecomment-1114360247, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO6IHZAGRTQJIQVEBMEO7HTVH4E5HANCNFSM5EOOQVBA . You are receiving this because you were mentioned.Message ID: @.***>

[Afroboltski]

The change control in this pr is a huge mess lol

Is there a way to reset the change tracking somehow? Because I did commit 493d48159c80e6b4588cad75aa513a0919baa13b which was literally just copy/paste the master branch back in, but unfortunately it not contains ALL changes that EVERYONE has done since the start of this PR... I was hoping GH would automatically recognise that there were 0 differences between this branch and the master at that point. Then commit 91e6e33ece5882a03e449738cab6e997ee9e66ec has all the AC stuff re-integrated.

Or just start a new PR?

[shiznit304]

I would make a whole new branch based off of the latest master and try merging in individual commits by cherry-picking. I might even try because I could use a feature here that can't be solved with programmable outputs. A/C tries to run when engine is off, thus making the car more difficult to start

On Sun, May 1, 2022 at 6:32 PM Afroboltski @.***> wrote:

The change control in this pr is a huge mess lol

Is there a way to reset the change tracking somehow? Because I did commit 493d481 https://github.com/noisymime/speeduino/commit/493d48159c80e6b4588cad75aa513a0919baa13b which was literally just copy/paste the master branch back in, but unfortunately it not contains ALL changes that EVERYONE has done since the start of this PR... I was hoping GH would automatically recognise that there were 0 differences between this branch and the master at that point. Then commit 91e6e33 https://github.com/noisymime/speeduino/commit/91e6e33ece5882a03e449738cab6e997ee9e66ec has all the AC stuff re-integrated.

Or just start a new PR?

— Reply to this email directly, view it on GitHub https://github.com/noisymime/speeduino/pull/665#issuecomment-1114367390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO6IHZG5B3SZHJMB7BQEKIDVH4H75ANCNFSM5EOOQVBA . You are receiving this because you were mentioned.Message ID: @.***>

[shiznit304]

What is your username on Slack?

[Afroboltski]

I would make a whole new branch based off of the latest master and try merging in individual commits by cherry-picking. I might even try because I could use a feature here that can't be solved with programmable outputs. A/C tries to run when engine is off, thus making the car more difficult to start

Heyyyyyyy wait a minute, the "Files Changed" tab now only shows the differences from this branch to the master, it seems Github HAS done exactly what I expected? Or did you do something in the background?

What is your username on Slack?

I'm on the discord now, user name is @Afroboltski

[Afroboltski]

Ahh bugger, I've found a whole bunch more merging errors. Lemme try and fix it

[floods57]

Hi again ;) Can't get it to work, no problem for upload it, but can't get it to work, IDLE up work fine (input and output) but not AC control, i try many different way, the AC always stay on (ouput on) and don't change if i ground the input (i try reverse polarity, but same), it is like it don't look at the input. My input is not the issue, i try the same input for IDLE Up and it work perfectly. The TPS work, i choose 95%, when i go over 95%, it cut the AC ouput and my stepper IDLE motor move right at this moment, same for RPM that ok. Other issue, randomly, when i open the AC control tab, all get wrong, it start to put on my boost ouput, and some time the VVT ouput and the stepper motor move nonstop in and out ! must put the AC control to off, restart, and switch it on again ! And i see a other issue, the fan control don't work correctly, (AC control off or on), it switch on according to the temp, but never switch off if temp are back down. I have a 0.4.3c board, all the rest work well. Thanks

[Afroboltski]

Hi @floods57

I found another line that I had removed, but the auto merge got confused and added it back in. This would have kept the fan on I think.

Remember the fan is supposed to turn on if there is an A/C demand, or temp is high. Otherwise turn off but only if the temp is lower than the off limit. We don't want the A/C to ever DEMAND the fan off, because it's primary function is cooling.

Regarding the A/C request being stuck on - can you try it with the button on other pins? I had the same problem during testing, turns out if the pin you are trying to use for A/C request is assigned in the programmable I/O page, it stops the input pullup resistor from being enabled. By the way, NORMAL polarity means the request will be ON if the pin is pulled to ground. REVERSE polarity disables the internal pullup, and it this scenario you would use an external pull-down resistor.

I'm not sure why opening the A/C page would screw things up, although I haven't fully tested the timing changes I've made yet.

[floods57]

Hi Afroboltski

Ok, should be ok for the fan.

For the A/C stuck on, i try different pins, but always same, i check to be sure the pins i use is not in the firmware init file, i try by removing the pin 48 from the pin mapping in the firmware, i use it for IDLE UP and try with it for the AC, but same issue, so i try with pin 51 (is my clutch input i don't use), remove it from the pin mapping too, it work with IDLE UP now or any other use, but still same issue with the AC control. I use normal mode (to ground).

Thanks

[Afroboltski]

Hi @floods57,

OK, let me run another test tonight and check.

[floods57]

Hi @floods57,

OK, let me run another test tonight and check.

Ok, thanks ;)

[FazxFi]

@floods57 If you want it fast you can use mine, It base on this code also, just a lot of changing/ ;p I think it work at least the pin reading problem https://github.com/HazuTo25/speeduino/tree/A/C-System_Custom

If it doesn't work either feel free to share.

If you can wait for this one get figure out, feel free.

[shiznit304]

I’ll test the latest PR with my ua4c in my daily driver in 7 hours or so.

On Tue, May 3, 2022 at 11:06 AM FZL @.***> wrote:

@floods57 https://github.com/floods57 If you want it fast you can use mine, I think it work at least the pin reading problem https://github.com/HazuTo25/speeduino/tree/A/C-System_Custom

If it doesn't work either feel free to share.

If you can wait for this one get figure out, feel free.

— Reply to this email directly, view it on GitHub https://github.com/noisymime/speeduino/pull/665#issuecomment-1116272985, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO6IHZEOGG5CE2MWDBFTDFDVIFFJ7ANCNFSM5EOOQVBA . You are receiving this because you were mentioned.Message ID: @.***>

[floods57]

@floods57 If you want it fast you can use mine, It base on this code also, just a lot of changing/ ;p I think it work at least the pin reading problem https://github.com/HazuTo25/speeduino/tree/A/C-System_Custom

If it doesn't work either feel free to share.

If you can wait for this one get figure out, feel free.

Thanks, i just try it, and all is working great, the only little "error'" i see it the TPS value that is not %, for 95% i have to enter 190, but that really minor, other than that all is working well.

I’ll test the latest PR with my ua4c in my daily driver in 7 hours or so. …

Ok, i try the last modification, and now the fan is working right, but all the rest get the same issue.

[Afroboltski]

..the only little "error'" i see it the TPS value that is not %, for 95% i have to enter 190, but that really minor, other than that all is working well.

We've fixed that in this PR now.

[floods57]

..the only little "error'" i see it the TPS value that is not %, for 95% i have to enter 190, but that really minor, other than that all is working well.

We've fixed that in this PR now.

Yes, in you version it was right in % Did you run a test and get the same issue i get ?

[Afroboltski]

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!

[FazxFi]

@floods57

Thanks, i just try it, and all is working great, the only little "error'" i see it the TPS value that is not %, for 95% i have to enter 190, but that really minor, other than that all is working well.

yeah, that was my bad. https://github.com/HazuTo25/speeduino/commit/fadde9d41df0adcdf4685aff275db34f8c66bee5 in .ini file mistake was done, I don't remember how it change to something else

[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

[Corey-Harding]

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

[transi01]

Have done some tests with your code and so far it works as it should :-)

I would suggest the following addons if possible:

Setting grid for the "min RPM for A/C operation" to a shorter range than 100RPM to eg. 20 or 50RPM?!

And maybe the implementation of a dedicated delay only for the fan (A/C fan and or std fan) like the delay for the compressor but independantly, to be able to first switch the fan on and then after another delay switch the compressor on. I think that could make sense :-)

// Thomas

[Afroboltski]

Have done some tests with your code and so far it works as it should :-)

I would suggest the following addons if possible:

Setting grid for the "min RPM for A/C operation" to a shorter range than 100RPM to eg. 20 or 50RPM?!

See the latest commit, it's now adjustable in 16rpm steps. 16 because that simplifies to bit shifting the main RPM signal which is potentially faster to calculate in the firmware. :)

And maybe the implementation of a dedicated delay only for the fan (A/C fan and or std fan) like the delay for the compressor but independantly, to be able to first switch the fan on and then after another delay switch the compressor on. I think that could make sense :-)

Why would you need an extra delay? You press the A/C button in your car and the fan comes on straight away, then the compressor engages after a delay. Or, after a high RPM/TPS condition, there is a lockout delay which you could also increase/decrease. What is the purpose of delaying the fan start further?

[transi01]

And maybe the implementation of a dedicated delay only for the fan (A/C fan and or std fan) like the delay for the compressor but independantly, to be able to first switch the fan on and then after another delay switch the compressor on. I think that could make sense :-)

Why would you need an extra delay? You press the A/C button in your car and the fan comes on straight away, then the compressor engages after a delay. Or, after a high RPM/TPS condition, there is a lockout delay which you could also increase/decrease. What is the purpose of delaying the fan start further?

I have done the tests while using the std. cooling fan for A/C testing and I'm pretty sure that the fan did step in at the same moment as the A/C compressor did. Can not initiate another test before the end of next week cause I have sisassembled the Header. If the special A/C fan will be switched on immidiatly after "A/C in" signal appears and the compressor steps in later then it's OK :-)

Thanx for changing grid for RPM limit setting :-)

[Afroboltski]

I have done the tests while using the std. cooling fan for A/C testing and I'm pretty sure that the fan did step in at the same moment as the A/C compressor did. Can not initiate another test before the end of next week cause I have sisassembled the Header. If the special A/C fan will be switched on immidiatly after "A/C in" signal appears and the compressor steps in later then it's OK :-)

Yeah the fan trigger (for the cooling fan), the dedicated A/C fan, and idle up are initiated as soon as the A/C request is received, prior to the comp delay.

Thanx for changing grid for RPM limit setting :-)

No worries :)

[shiznit304]

Good stuff. I’ll check it out too.

On Sun, May 8, 2022 at 3:14 PM Afroboltski @.***> wrote:

I have done the tests while using the std. cooling fan for A/C testing and I'm pretty sure that the fan did step in at the same moment as the A/C compressor did. Can not initiate another test before the end of next week cause I have sisassembled the Header. If the special A/C fan will be switched on immidiatly after "A/C in" signal appears and the compressor steps in later then it's OK :-)

Yeah the fan trigger (for the cooling fan), the dedicated A/C fan, and idle up are initiated as soon as the A/C request is received, prior to the comp delay.

Thanx for changing grid for RPM limit setting :-)

No worries :)

— Reply to this email directly, view it on GitHub https://github.com/noisymime/speeduino/pull/665#issuecomment-1120480562, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO6IHZGEQW656AWESSOOZKDVJAOCDANCNFSM5EOOQVBA . You are receiving this because you were mentioned.Message ID: @.***>

[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!