It is currently Sun Jun 24, 2018 1:29 am



Forum locked This topic is locked, you cannot edit posts or make further replies.  [ 31 posts ]  Go to page 1, 2  Next
Author Message
 Post subject: refactoring ideas
PostPosted: Wed Aug 21, 2013 12:40 am 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
There are some cool algorithms in this code base, I really like what you guys are doing but I think the implementation could be simplified a bit. I am Russian, it's hard for us Russians to be nice, but I will try to do my best.

So, let me try with an example. "void Get_Prime_Corr"
1) If the method is called "Get...", I think it should return something :) A method which is "void" should probably be called "Calculate_..."

In our particular case, I think it would be better to start returning the value. The way code is right now it depends on global variables too much, I know these global variables are needed to return state to the tuner - but within the code, it would be better to establish more explicit data float, thus methods returning values into local variables instead of methods talking to each other via global variables. Global - bad :)

2) "Prime_Corr = table_..; Prime_Corr = 1+ (Prime_Corr * Prime_Decay);"
"x = a; x = x * b; x = x + c;" is a typical pattern. I think something has to be changed here: "x = a" is not 'x' yet - it's just the first step of the logic, it's not the real value and would never have the meaning expected if you consider variable name. Only the final result deserves to be called 'Prime_Corr', and only this case one reading this code has a chance to figure out the logic of calculating Prime_Corr. That's probably a messy explanation, but I have tried my best.

3) three levels of nested 'if's smell wrong. Let's extract a method to handle 'Prime_Decay' logic, and by doing so we actually get a much nicer-looking logic of 'Prime_Corr' calculation.

Code:
                float value = table_lookup(CLT, 1, Prime_Corr_Table) * Inverse100;

                // apply the decay
                result = 1+ (value * Get_Prime_Decay());


I have pushed this change into my https://code.google.com/r/arro239-open5xxxecu/ clone. I am not a huge git user and I do not see there the "pull request" button is, but if you guys are interested I would love to contribute some more refactoring like that. I believe that if code design gets improved a little, there would be hope for this code to be much more readable and much more reusable.

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Aug 21, 2013 2:44 am 
Offline
User avatar

Joined: Sat May 11, 2013 9:52 am
Posts: 304
Location: Over here, doing 'over here' things.
Don't worry much about being 'nice'.
Sometimes things require a different perspective.

Just be honest.

_________________
/me goes off to the corner feeling like Jerry Springer with a mullet.

My O5E candidate: 1982 Honda CX500TC motorcycle.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Aug 21, 2013 7:51 am 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
Your suggestions sound correct to me.

I wrote most of the actual engine control stuff and I'm absolutely NOT a programmer so it comes as no surprise at all there are better or more standard ways to accomplish what I was after.

I added you you the o5e googlecode project so you should now be able to commit to the project repository. May be create you own branch?

What does need to happen with these changes is we need to confirm the outputs are correct before the changes get moved on an official branch. Do you plan to get a board or will we need to find someone who does have a board to help with the testing?

BTW, my wife is Ukrainian and shares your preference for "lets say getting straight to the point and avoid confusion". She has softened a bit over the years but I used to hear Вы понимаете? (do you understand?) quite often....to which the ONLY acceptable answer was "Да, дорогая" (yes Dear) :)


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Aug 21, 2013 11:34 am 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
mk e wrote:
May be create you own branch?

What does need to happen with these changes is we need to confirm the outputs are correct before the changes get moved on an official branch. Do you plan to get a board or will we need to find someone who does have a board to help with the testing?

I am probably not getting myself a board yet, that would be something I will think about later.

Thank you for the RW permission, I've created a branch for myself. Either someone would need to test the code, or I can suggest another approach:
I am used to making a lot of tiny "atomic" changes and I usually put nice commit messages, that way one should be able to review and validate them one by one. With a dozen of such atomic changes we can move the code into any direction, hopefully into the right direction :)

So, I will now commit another tiny change into my branch. Please have a look and see if you think it would be possible to just code-review them. No pressure :)

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Aug 21, 2013 2:20 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
I looked over you changes and they looked right and built properly......but I do want to actually test it before moving anything off your personal branch.

We can figure something out for testing like you let me know when a section is complete and I'll fire it up and if it works right push it up to development.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Aug 21, 2013 6:31 pm 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
Problem:
Looks like 90% of 'Inverse100' magic constant usages happen in connection with 'table_lookup' method. Also, there is too much of single-dimensional lookup in the form of 'table_lookup(CLT, 1, Prime_Corr_Table)', sometimes it's both. Sometimes the table value has to be added to '1' to get a meaningful coefficient.

I suggest extracting tiny methods to separate value lookup from actual logic. So, for example
Code:
Corr = table_lookup(RPM, Reference_VE, Inj_Time_Corr_Table);
        Corr = 1.0f + (Corr * Inverse100);

would turn into
Code:
static float Get_Main_Fuel_Corr(float rpm, float reference_ve) {
   // Main fuel table correction - this is used to adjust for RPM effects
   float table_value = table_lookup(rpm, reference_ve, Inj_Time_Corr_Table);
   return 1.0f + (table_value * Inverse100);
}

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Aug 21, 2013 6:56 pm 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
mk e wrote:
We can figure something out for testing like you let me know when a section is complete and I'll fire it up and if it works right push it up to development.

I've extracted a couple more methods - I think there should be enough to test.

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Thu Aug 22, 2013 8:41 am 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
russian wrote:
Problem:
Looks like 90% of 'Inverse100' magic constant usages happen in connection with 'table_lookup' method. Also, there is too much of single-dimensional lookup in the form of 'table_lookup(CLT, 1, Prime_Corr_Table)', sometimes it's both. Sometimes the table value has to be added to '1' to get a meaningful coefficient.


Yes. This is a by-product of the standardized variables. Originally I set up TS to work this magic for us but that lead to all control algorithms in both TS and the FW needing to be custom/task specific.

The move to floats and a "watch you see is what you get" approach allows much more generic treatment or everything but there was a price to be paid and that price was the need to convert values that the user enters as a % int a correction factor buy dividing by 100 and adding 1. Maybe there is another option, but nothing popped out to me......thoughts?

Clearly I didn't comment this adequately.....I'll get that on my to-do list so at least it will be more clear what all the conversions are for (if they stay).


On the 1d tables, I'm not sure I understand the question there? May things in the world are simple x-y relationships but are non-linear so there are a lot of 1d tables. What are you thinking here?




russian wrote:
I suggest extracting tiny methods to separate value lookup from actual logic. So, for example
Code:
Corr = table_lookup(RPM, Reference_VE, Inj_Time_Corr_Table);
        Corr = 1.0f + (Corr * Inverse100);

would turn into
Code:
static float Get_Main_Fuel_Corr(float rpm, float reference_ve) {
   // Main fuel table correction - this is used to adjust for RPM effects
   float table_value = table_lookup(rpm, reference_ve, Inj_Time_Corr_Table);
   return 1.0f + (table_value * Inverse100);
}


I don't have any objection really but I'm not sure I'm seeing/understanding the improvement? Help me understand this one Andrey.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Thu Aug 22, 2013 9:10 am 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
It's all about simplicity. I am not the one who has invented software design principles but I've realized that most of them help me a lot because they make my job easier.

In the ideal world, one class has one purpose. If we are less perfect, at least one method should have one clear purpose.
So, instead of "this method knows how to convert TS tables and calculate fuel pulse width coefficients of thee different natures and it also schedules actual fuel squirts", I tend to divide and conquer and have a number of different methods. One for each coefficient, one for overall math and a separate one for scheduling.

A rule of thumb: a human can digest a method of the length of one screen. As soon as start scrolling up and down, you loose focus. So, by taking the "1 + x / 100" logic somewhere else, I am saving myself from the hassle of paying attention to how the coefficient is acquired. All I care is that you have a coefficient, and it's nice to know that say CLT and RPM are inputs of the get_coeff method, and whatever happens inside the me method I do not care. If I would want to look at it, I would look into it separately - but I do not need while I am looking at the bigger picture. So, all this method extraction is about separating the "layers" of the picture: you look at the general method and you see the general idea, if you are looking for implementation details you look inside the smaller methods. With a proper IDE while allow you to go into methods and back that's easy (BTW CW 2.10 looks like it is older then I am, I've used eclipse to navigate around the code and only used CW to validate that everything still compiles, but let's not get off track)

As for lookup(clt, 1, table) - it's more or less the same idea. The fact that you have implemented f(x) as a special case of f(x, y) where y==1 is an implementation detail. Ideally, you should hide this implementation detail - if you can. Since we do not have http://en.wikipedia.org/wiki/Function_overloading - maybe all we need is a plain method
Code:
float lookup_2d(x, table) {
return lookup_3d(x, 1, table);
}

because if we do not do that, "1" is now a magic constant - http://en.wikipedia.org/wiki/Magic_numb ... _constants

Do not get me wrong - I am not for perfection just for the sake of perfection. As soon as you follow at least some of these principles and your code is good enough, it is good enough. For different people the level is different, but the main difference is probably between the original author and any outsider. The author is used to the code and he knows the history and context. An outsider like me does not, it has to be simpler for the sake of the outsider.

Wow, that's a lengthy response :)

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Thu Aug 22, 2013 9:19 am 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
russian wrote:
It's all about simplicity.

......

Wow, that's a lengthy response :)


Long, but helpful and I see your point. Change away ;)


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Tue Oct 29, 2013 6:52 pm 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
Poke. Let's merge these changes so that I can make more changes :)

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Oct 30, 2013 3:54 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
russian wrote:
Poke. Let's merge these changes so that I can make more changes :)


Let me see what I can do. I've got ferrari porting on my brain and having trouble getting back to code.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Fri Nov 15, 2013 1:03 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
russian wrote:
Poke. Let's merge these changes so that I can make more changes :)


Ok , I'm on it. Give me a couple days though as I move my brain back into code mode.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Fri Nov 15, 2013 3:56 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
mk e wrote:
russian wrote:
Poke. Let's merge these changes so that I can make more changes :)


Ok , I'm on it. Give me a couple days though as I move my brain back into code mode.


I forgot I had an ini issue to deal with on the user output stuff. I need to fix that first then I'll merge your stuff.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Mon Nov 18, 2013 4:55 pm 
Offline
User avatar

Joined: Mon Nov 18, 2013 3:36 pm
Posts: 2
Hello,

I'm absolute novice to the forum and project, but I have some ideas to share about code re-factoring.

First of all, thanks for all efforts to put all this together and make it work.

I like KISS - "Keep It Simple, Stupid" code strategy. Simple is better when it came to code live(fixes, updates adding features, etc.) and support.

The Russian is right and I'll support him in this endeavor.

I have one more idea - object oriented C code. I believe this is the right way to go. This project will grow up and the only way to keep it under control is to put all data and functions in logically connected structures.

I'll give one short example:
Code:
#include <stdio.h>

#define STR_SIZE 8
typedef struct {
   char name[STR_SIZE];
   float value;
   char unit[STR_SIZE];
   char format[STR_SIZE];
} float_t;

void main (void)
{
   float_t rpm = {.name="RPM", .unit="RPM", .format="%5.0f", .value=900.f};
   char val[STR_SIZE];
   snprintf(val, STR_SIZE, rpm.format, rpm.value);
   printf("%s=%s[%s]", rpm.name, val, rpm.unit);
}

The code above is just for demonstration of object oriented C code.
Objects that are missing right now are the big ones - engine, camshaft, crankshaft, etc.

I have some more ideas about data tables importing from stock ECUs (multiple modes - stock and god), anti-lag, couple stages of injector, etc.

Most important thing, according to me, is to put good code base and leave the other to build over.
In this relation I think we need one more hardware abstraction layer to "hide" all hardware specific things. This layer will allow project porting to other hardware platform one day.

I'll be happy to be part of this project.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Tue Nov 19, 2013 3:10 pm 
Offline
User avatar

Joined: Mon Nov 18, 2013 3:36 pm
Posts: 2
Hello,

I have couple suggestions related to C code - objects and no more magic numbers.

I'm new to the project, but I'm here to help with the code. In day or two I'll have building environment. Until then I'm open to discussion about coding strategies.

Great project!


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Wed Nov 20, 2013 12:02 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
dobrin,
I missed approving you posts! They should go straight up now.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Thu Nov 21, 2013 9:09 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
dobrin wrote:
Hello,

I have couple suggestions related to C code - objects and no more magic numbers.

!

i know nothing about object....not a programmer but I'm certain the magic number was not only a bad idea for programming it almost certainly would cause mystery bugs too when a variable +magic number and the code did the wrong thing.

I'm concerned to about just how fragile the code is.....bad table value = crashed code. that really should be fixed.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Mon Nov 25, 2013 4:37 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
russian wrote:
Poke. Let's merge these changes so that I can make more changes :)


merged.


.......we lost fuel. :o

I'll see if I can figure out where it's gone.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Mon Nov 25, 2013 4:44 pm 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
mk e wrote:
.......we lost fuel. :o

I'll see if I can figure out where it's gone.

Did I break it? How do you test it?

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Mon Nov 25, 2013 4:57 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
russian wrote:
mk e wrote:
.......we lost fuel. :o

I'll see if I can figure out where it's gone.

Did I break it? How do you test it?


Yes. :)

I test on a 5634 TKR development board.


it's appears to be stuff like this:
Code:
static float Get_Main_Fuel_Corr(float rpm, float reference_ve) {
   // Main fuel table correction - this is used to adjust for RPM effects
   float table_value = table_lookup(rpm, reference_ve, Inj_Time_Corr_Table);
   return 1.0f + (table_value * Inverse100);


where new and meaningless variables "rpm" and "reference_ve" are created

The real variables are "RPM" and "Reference_VE".. I'll go through and swap them all out and see what I get.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Mon Nov 25, 2013 5:01 pm 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
mk e wrote:
russian wrote:
mk e wrote:
.......we lost fuel. :o

I'll see if I can figure out where it's gone.

Did I break it? How do you test it?


Yes. :)

I test on a 5634 TKR development board.


it's appears to be stuff like this:
Code:
static float Get_Main_Fuel_Corr(float rpm, float reference_ve) {
   // Main fuel table correction - this is used to adjust for RPM effects
   float table_value = table_lookup(rpm, reference_ve, Inj_Time_Corr_Table);
   return 1.0f + (table_value * Inverse100);


where new and meaningless variables "rpm" and "reference_ve" are created

The real variables are "RPM" and "Reference_VE".. I'll go through and swap them all out and see what I get.



hmmmm....can't just swap them :cry:


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Tue Nov 26, 2013 12:02 am 
Offline
User avatar

Joined: Fri Aug 16, 2013 12:04 pm
Posts: 57
Location: Jersey City, NJ
mk e wrote:
where new and meaningless variables "rpm" and "reference_ve" are created

The real variables are "RPM" and "Reference_VE".. I'll go through and swap them all out and see what I get.


Wait a sec, these are NOT variables - these are method parameters! While this method is invoked, the variables are passed as parameter values.

Anyway, I could have very well broke something :( So if you are testing with the board I assume you have some synthetic signal which causes the code to run?

I will review the diffs tomorrow, hopefully I would be able to just see the issue. Please do not rollback this yet, let's see if we can get figure out where the bug is exactly.

_________________
another open ecu


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Tue Nov 26, 2013 3:47 am 
Offline
User avatar

Joined: Sat May 11, 2013 9:52 am
Posts: 304
Location: Over here, doing 'over here' things.
C versus C++ issues?

_________________
/me goes off to the corner feeling like Jerry Springer with a mullet.

My O5E candidate: 1982 Honda CX500TC motorcycle.


Top
 Profile  
 
 Post subject: Re: refactoring ideas
PostPosted: Tue Nov 26, 2013 5:45 am 
Offline
User avatar

Joined: Sat May 11, 2013 9:45 am
Posts: 729
Location: PA, USA
russian wrote:

Wait a sec, these are NOT variables - these are method parameters! While this method is invoked, the variables are passed as parameter values.


What sets their values? rpm = ?????

RPM = (float)fs_etpu_eng_pos_get_engine_speed(etpu_a_tcr1_freq);

...so it's actually RPM .


russian wrote:
Anyway, I could have very well broke something :( So if you are testing with the board I assume you have some synthetic signal which causes the code to run?


We have quite a bit of self-testing built in.

There is crank simulator that Generates what crank signal the the user has selected for their input that can be jumped to the input. There is a TS menu to set the RPM and select fixed RPM for most stuff or 1 or several configurable patterns to do dynamic response type testing.

There is a 0-5V pot on the board that can supply a voltage

There is a menu to allow a TS entered value to replace any input voltage the ADC sees

There is another menu to allow a TS entered value to replace any input value (the post calibration table number) in the code.

There are LEDs that can do quick visuals if output are where they say they are

TS can read, display, and log pretty much any output

....so pretty much anything can bench be tested with just the board and a PC

I also keep a small scope handy in I need to confirm anything independently.


russian wrote:
I will review the diffs tomorrow, hopefully I would be able to just see the issue. Please do not rollback this yet, let's see if we can get figure out where the bug is exactly.


I did not push it up....should I?


Top
 Profile  
 
Display posts from previous:  Sort by  
Forum locked This topic is locked, you cannot edit posts or make further replies.  [ 31 posts ]  Go to page 1, 2  Next

Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Theme designed by stylerbb.net © 2008
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group
All times are UTC - 5 hours [ DST ]