search

sim- / tgy

tgy -- Open Source Firmware for ATmega-based Brushless ESCs
http://0x.ca/tgy/
686 stars 388 forks source link

made the following settings configurable via *.inc #96

Open msperl opened 8 years ago

msperl commented 8 years ago
ahalekelly commented 8 years ago

I'm a little confused. Personally I think that the hardware and board specific settings should be in the .inc files and the general configuration and personal preference settings should stay in the main tgy.asm file. But if we're making the general settings configurable with the .inc files, shouldn't we allow that for all of them? Why just these settings?

msperl commented 8 years ago

I started with the things I needed - we can easily add the others as well

ahalekelly commented 8 years ago

When I need to change settings like this, I change the tgy.asm file locally and recompile. Would that not work for your application? Also, adding this to the main repository only makes sense if one of the *.inc files makes use of this, which wasn't in your pull request.

msperl commented 8 years ago

I guess there are a few "out of trees" *.inc (e.g: blueesc) that would probably profit from this and avoid having to locally modify the tgy.asm, which IMO is not very intuitive.

ahalekelly commented 8 years ago

In order to make it intuitive and easy to configure, we should have all the configuration settings that someone would need in one file. Having to edit two files, or worse yet, editing one file and having it overwritten by the other, is too confusing.

So if people wanted to configure only from the .inc file, we could copy all the settings to every .inc file and delete them from tgy.asm. Right now, normal users shouldn't ever have to edit the *.inc files, and they can do everything through the tgy.asm file, which I personally think is just as intuitive. Or we could break them out into a separate settings file.

ahalekelly commented 8 years ago

Now I understand what you were saying, that having the configuration in the tgy.asm file poses a problem for forked versions, because when a new version of the underlying code in tgy.asm is released, they have to merge that into their own edited version of tgy.asm. A separate centralized settings file would make this easier, in that they could just replace their tgy.asm file with the new version and not have the settings change.

msperl commented 8 years ago

you may put it that way... I in my personal branch only modify tgy.inc to set: RC_PULS_REVERSE and BEACON_IDLE

Changes to the *.inc are rare by comparisson (tgy.inc had the last real change 2 years ago, while tgy.asm changes quite more often)

ahalekelly commented 8 years ago

Yeah, from the perspective of the forked repos, moving the configuration out of tgy.asm makes a lot of sense. I would be happy to make a pull request for a separate config.inc file, as it would make it easier for both forked repos and people using this one. It would also be a good opportunity to make the documentation of the various options better. However I believe it would break compatibility with some flashing tools like RapidESC and KKMulticopterFlashTool. Could we release a beta first and get those developers to support it before putting it into the master branch?

msperl commented 8 years ago

Why do you want to keep it in another config?

For example the blueesc needs reverse enabled as it is geared towards (u)boats, so why not keep thosesettings in the corresponding *.inc?

The change I had proposed are not impacting others as there is still the defaults that apply the same as before, so these would stay transparent...

msperl commented 8 years ago

Why I need it? I want to reduce the idle power consumption of the ESC from 50mA to <30mA (at least my tgy and similarly also with the blueesc), but I want to have custom settings (like the above) without having to always remember about reverting those settings to the defaults before submitting patches - I already got it down to 40ma but see prospect of moving it down to 35mA (and with inverted RC on INT0 down to 29mA).

I actually thought about your argument as well and I think that I have a "flexible" solution that works mostly on the makefile and that does not require a change to tgy.asm for an addiional board, but still retains the defaults when unset in tgy.asm.

msperl commented 8 years ago

added more .if !defined() to make more values overrideable via board.inc...

ahalekelly commented 8 years ago

My concern is mostly for the people who don't have their own forked copy. Right now the process of flashing a board is that you download the latest version, edit the desired settings in tgy.asm, compile and flash. Most people never have to open the .inc file. If some of the settings they set in tgy.asm are unknowingly being overridden by the .inc file, it would be very confusing and frustrating, and this is why we can't have two different configuration files. We have to choose one file to be the main configuration file, whether that be tgy.asm, *.inc, or a seperate config file.

If we make every setting accessible from the .inc files, we would have to start telling people to never touch the tgy.asm file and do all the configuration through the .inc files. That seems to me like a hassle without much gain. You can already reverse the motor direction without modifying tgy.asm, either by physically swapping two motor wires or by swapping phase A and C in the .inc file. For the other settings, it's pretty easy to just add the .inc file to a git commit and not add tgy.asm with your modified settings.

msperl commented 8 years ago

The way it is now you first have two places to define things for some parameters:

My concern is that with local tgy.asm changes the settings apply to all boards and I wonder if this is really sensible - if I need to change the deadtime right now, I would need to change it in tgy.asm and that would immediately translate to all boards on the next full make - not a good idea if you build for multiple boards...

This patch just extends the possible settings that you may modify in *.inc to more parameters (still not compete).

People may still change the settings in tgy.asm if they like (but I would discourage it), we would just give them a different place they could use!

Together with the change to the makefile the steps for a custom hex with custom settings could be:

Then "make my.hex" and you get your personal hex file... Switching to a newer version of the firmware becomes a simple "git pull" and "make my.hex" - no more need for merging!

This helps also the developers (at least myself) as I can now have my personal test configs for multiple scenarios in one go and I do not need to modify the configs to test each variant. If you look at my sleep patch I can then create 2 additional files, say tgy-sleep=1.inc and tgy-sleep=2.inc and with a simple make I can see that all variants work...

ahalekelly commented 8 years ago

That's a really elegant solution.

The way it works now, things like complementary pwm and deadband are rightly configurable from the esc.inc file, while other things have to be configured from tgy.asm, which is exactly what I said I didn't want. With your suggestion, basic users can simply locally add the settings they need to the existing *.inc file, and advanced users can have separate configuration files for different uses with the same ESC.

It should be backwards compatible with software like RapidFlash and KKMulticopter too right? I'll test it and then write up a commit to make all the settings configurable from the *.inc files and put a message at the top of tgy.asm saying that if you want to change the configuration, change the applicable esc.inc file or create a new my.inc file.

msperl commented 8 years ago

Compatiblity: i guess these tools just focus on hex files, so as long the hex files are identical it should not prove to be an issue.

As for a new patch: why don't you merge the patches that does all of it. Those loose ends I did not make modifiable you can add if we need it.

As for documentation: I would recommend an example inc file that has all the settings including a detailed description...

msperl commented 8 years ago

@ahalekelly:I created a merge request for the make-file changes against your master branch for easy application of the patch (https://github.com/ahalekelly/tgy/pull/1) The config patch (https://github.com/ahalekelly/tgy/pull/2) does not apply because of your changes in https://github.com/ahalekelly/tgy/commit/a654f7db862dea24549c0dbe08ebe27950bfc484

kh4 commented 8 years ago

I am the maintainer of the web based firmware builder the RapidFlash (chrome) tool uses.

It currently parses the tgy.asm file and gathers all the .equ statements, it will be quite easy to change it to parse other files but the change would be incompatible and some additional logic would be needed to handle both cases depending on fw version.

msperl commented 8 years ago

well, unfortunately there are no fine details of your web-app so that we could support it better, but in the end it should make your life much easier, as now you only have to create a temporary file with your "required .equ" and then you build that corresponding hex with no issues/changes to the source-code.

So you could keep the old code, but with an identifier you could just as easily switch to the new parser/executor.

Not knowing the fine details (the webapp not being available on github) I can only guess that it is not that complex and it should really help you streamline the system.

Ideally we could even create a documentation template for all the typical parameters that you may want to change, and which you may want to include/parse in your code directly...