New EEPROM code and various code quality fixes

[giacomo892]

• New clear and more maintainable EEPROM code, 100% backward compatibile.
• Code size reduction (-2.1% of FLASH usage)
• Various bugs and programming issues fixed.

Thanks for the help to @shellixyz

[giacomo892]

Currently NOT safe to use. No idea why yet.

[AlkaMotors]

What bugs are fixed here?

[shellixyz]

At least one extern variable alignment bug causing a hard fault with the GCC version we are using. Also updating the firmware name

[AlkaMotors]

OK, so elaborate a bit.. what was wrong with updating the firmware name.. what variable and what version of gcc? I have not had those issues

[giacomo892]

Hi @AlkaMotors the alignment issue @shellixyz is talking about was caused by me, but being "extern" used incorrectly the compiler wasn't warning me till I found the culprit. Regarding the firmware's name in the EEPROM the code wasn't checking for it to be changed so when testing for targets on an ESC you ended up with the wrong name being displayed. I still need to flight test the firmware, that's why I've marked it as a draft for the moment

[AlkaMotors]

OK, no problem i will fix the name being updated. Right now its only updated on Version number change. I won't be changing any variable names though like "signaltimeout" to "input_signal_missing".. I just don't see a reason too.

[giacomo892]

Well man, don't focus on a single variable name, this PR is much more than that! It makes the code clearer and more functional overall! Regarding the name, you don't need to fix anything, this PR fixes the issue.

I invite you to try this branch on the bench and tell me if you find something not working. Thanks!

[DusKing1]

@giacomo892 @shellixyz nice work, cleaner code!

[AlkaMotors]

I am not focused on one variable name.. many of them have changed. The fix for the name update takes only a second. Why would i do an update then spend my time looking for something not working... when it all works now? Since there is only one person who ever changes the code i need it to be familiar and stable and not give me any suprises

[giacomo892]

All good. You can do whatever you want with this PR and related code. Won't push it any further.

[shellixyz]

@AlkaMotors We haven't changed anything in how the code is working besides making the settings loading/saving cleaner, smaller and more maintenable while keeping the exact same memory structure for backward compatibility with the configurator. The rest is just moving the external definitions where they should be to also make the code cleaner, more maintenable and prevent type sync issues. When @giacomo892 said it was unsafe to use it was just a precaution, the issues he was having were unrelated to these changes in the end.

[AlkaMotors]

I like some of the changes. There is just a lot to consider, all of my st studio debugs are set up with the same variable names. If a different target is to be flashed its important to press 'send default eeprom' first then flash. This will also ensure that a new target will start in a safe hardware configuration and also make the name reset. The configurator will do this in the future automatically if a different target name is chosen, that would fix it for all previous versions of the firmware too. The problem is I do want some of these things.. just not all! It would be nice to discuss changes beforehand.

[shellixyz]

We can split the changes, that's not an issue. Not sure what your issue with the variable names are. Are you talking about predefined watchpoints ? If what's bothering you is the global variable names changes we can revert that.

Something which should definitely be merged and won't impact you is the move of the external definitions. I guess the settings loading/saving won't either.

[AlkaMotors]

I am not entirely sure.. I am new to this but my process to be able to debug easily has all the main variables as global variables so I can view and change them easily in debug environments like cube or st studio. I have probably a dozen or so saved configurations in st studio by variable name that I just open up and I gives me 30 or so live variables that I can see and edit, redoing all them work be a pain. The variables have the same names as the other github projects I have besides am32. Move of the external definitions? I dont know what that means but when I look it up I'll try back to you on that..

[shellixyz]

I think only signaltimeout has been renamed to input_signal_missing that's it. This can easily be reverted if you want.

The move of external declarations doesn't change anything for you. The variables are still global and available to the debugger like they were before with the same name. Extern declarations should be done in the header corresponding to the C file they have been defined into, that's the logical way to do it. For example if you define a variable in xxx.c the extern declaration should be put in xxx.h. Then you only need to include xxx.h in whatever file you need to use the variable from. It brings some type checks safety. If you have the variable declared as a different type in the external declaration compared to the definition the compiler will produce an error instead of having to trace a run-time failure. Also avoids duplication which is very bad for maintenability of the code.

[AlkaMotors]

I was looking at the things with "_compact" at the end.

[shellixyz]

That's just setting names. The settings are still the exact same and you can still display/access the settings struct as a raw int8 array if you want. But it is a lot better to have names for them than to access them with hardcoded indexes.

[AlkaMotors]

It looks like those names are used throughout the firmware though like in main.c things like 'if(use_sin_start){' are now 'if(settings.hardware.sin_startup)' . which is definitely not a hardware setting in the first place I wouldn't expect or look for it there. If I fire up st studio would it still work or would I have to change my variable to settings.hardware.sin_startup?

[shellixyz]

You would have to change your variable to settings.hardware.sin_startup. I don't know how @giacomo892 decided to split the settings structure. I guess it would be better in the common struct.

If you want people to contribute the code structure and formatting needs to be improved a lot. Right now it is close to unreadabe/unmaintainable apart from you perhaps because you wrote it. For example using hardcoded indexes and global variables for individual settings is a very bad practice. C is full of traps enough already, no need to make it more difficult than it inherently is.

[Voodoobrew101]

Just my 2 cents.

One thing I have noticed is Alka is more likely to incorporate smaller focused changes, For example if you want to move the EEprom defaults you should really only modify main.c and eeprom.c If you want to rename main.h and add a new main.h That Should be in a completely separate request that only adds the file and renames the other file as well as its links.

If you want to go to random files removing spaces, that should also be a separate request.

I could go through the kitchen cleaning up all kinds of stuff and my wife loves it, If anything gets moved though, my wife eyes burn with the fires from hell. People like things where they remember them being. That's not to say things can't be moved. But go for a little bit at a time.

Statements like this "Right now it is close to unreadabe/unmaintainable apart from you perhaps because you wrote it." maybe be gentler with your words. That's a bit harsh and not going to make any friends. Simply saying "trying to clean things up is understood."

[DusKing1]

I notice that this PR is on the slippery slope that nobody wants to be dropped in that direction.

For example if you want to move the EEprom defaults you should really only modify main.c and eeprom.c If you want to rename main.h and add a new main.h That Should be in a completely separate request that only adds the file and renames the other file as well as its links. If you want to go to random files removing spaces, that should also be a separate request.

Don't you check the comments above?

I could go through the kitchen cleaning up all kinds of stuff and my wife loves it, If anything gets moved though, my wife eyes burn with the fires from hell. People like things where they remember them being. That's not to say things can't be moved. But go for a little bit at a time.

That's off the topic, what's your point of sharing your personal lifestyle here?

Statements like this "Right now it is close to unreadabe/unmaintainable apart from you perhaps because you wrote it." maybe be gentler with your words. That's a bit harsh and not going to make any friends. Simply saying "trying to clean things up is understood."

Any vague comments on issues involving technical details can make things worse. Of cause we come here to make PR is to make the project we like become better.

So stop from off the topic and chaotic, come back to the PR and the code itself.

[giacomo892]

I was looking at the things with "_compact" at the end.

I added compact because you decided in the first place to store the settings in the eeprom as 8bit. So compact are the "compacted" values that will need a conversion process to be used in order to make them usable. See motor_kv for example.

Solution: store 16bit settings.

It looks like those names are used throughout the firmware though like in main.c things like 'if(use_sin_start){' are now 'if(settings.hardware.sin_startup)' . which is definitely not a hardware setting in the first place I wouldn't expect or look for it there. If I fire up st studio would it still work or would I have to change my variable to settings.hardware.sin_startup?

Copied the structure right from here:

https://github.com/AlkaMotors/AM32-MultiRotor-ESC-firmware/wiki/Open-ESC-EEPROM-Format

[AlkaMotors]

Ok, let's not worry about some off topic example .. doesn't bother me. This is a human problem. The issues were caused by my lack of knowledge experience when writing the firmware. The pr is technically really good that I can see. The part that is crucial though is that I must understand each change before its going to happen. There are a lot of things about this pr I really like. Like getting rid of 'extern' . Also some parts of the eeprom buffer changes. The changes to the code to access flash memory I am not sure about. Why delete the part where a page erase function is done only on a start of a page? Now it looks like it will page erase each and every time regardless of address? What will this do if we write a higher address than an even page?

[shellixyz]

Now it looks like it will page erase each and every time regardless of address? What will this do if we write a higher address than an even page?

That's because there is only one page to erase. The settings span only one page (pages are >=1KB). Each time you want to write the settings the page needs to be erased then the settings written. But you're rising a good point, the erase procedure should for good measure be put in the write loop so that if it happens in the future that settings span more than one page each page is erased first before being written to.

[AlkaMotors]

Yes, that's why I intentionally put it in there. For the eeprom setting it is ok but there are many other reason to write to a higher address within that page.

[shellixyz]

Right, save_flash_nolib I guess was intended before this PR as a more generic function. The way erase was implemented was flawed in any case though. Like now the code would only erase the first page. The only difference is that it would only do it if you happened to start writing at a flash page boundary. What do you need to write to flash besides settings ?

[AlkaMotors]

No that was intentional too, page erase should only be done on the base page address ( it might only be possible on that address anyway, I'll have to check the RM) Then the page is not erased again when additional parts of the page might be written later. If you do want to update/preserve other data you would have to read and rewrite anyway. Which automatically erases the page. Mostly I use it for diagnostic data, hall sensor learning info or other specific calibration.

[shellixyz]

Ah ok, makes sense

[AlkaMotors]

The other eeprom access changes are good .I really like the simplicity of memcpy when retrieving data from flash. I never even knew about that function until much later.

[AlkaMotors]

I really like this but the changes were just too much and altered some eeprom code that was done for a reason.