Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate the nameBuffer for freeing up additional RAM #207

Open
elral opened this issue Oct 6, 2022 · 7 comments · May be fixed by #219
Open

Eliminate the nameBuffer for freeing up additional RAM #207

elral opened this issue Oct 6, 2022 · 7 comments · May be fixed by #219
Assignees

Comments

@elral
Copy link
Collaborator

elral commented Oct 6, 2022

As discussed on Discord already the nameBuffer could be eliminated and instead of the name from an input device an "identifier" would be send. The connector have to know about the identifier and the relation to the input device name, therefore the user will not seen any differences. The benefit is that the nameBuffer stored in the RAM is not required anymore and additional RAM is available. Furthermore the serial traffic is reduced as the names could be "long" and instead just an identifier is transferred.

The identifier could be the x.th device from the same type which gets initialised. This information is already available from number of intialised decices, e.g. for the buttons buttonsRegistered in buttons[buttonsRegistered] = new (allocateMemory(sizeof(MFButton))) MFButton(pin, name);. Instead of saving the name buttonsRegistered would be saved and on an event transferred.
During processing the config there is always the same order of initialising input devices so there is also always the same indentifier.
Reading the config from the connector happens also in the same order, therefore the connector has a fix relation between the identifier (x.te device) and the name which is transferred during reading the config.
The same applies to all other input devices.

For backwards capability the connector has to check the firmware version and depending on this the connector has to expect names or identifiers.

@GioCC
Copy link
Contributor

GioCC commented Oct 6, 2022

I repeat here the comment I made on Discord about the choice of the ID.
Unless there's some more subtle implication I missed, using the Arduino pin number (data pin or first pin for multi-pin devices) as ID rather than the config order index would be much more convenient.
In the firmware, the reference pin is readily available, while the config order index should be stored just for this purpose, wasting memory.
Besides being as unambiguous as the index, it is also absolute: imagine reading the messages for troubleshooting and trying to figure out which "Button at position 15 in the internal array (or in the configuration sequence)" is, vs. "Button attached to pin #28".

@GioCC
Copy link
Contributor

GioCC commented Oct 6, 2022

For outputs, the config order is now more conveniently used as ID because it allows immediate addressing; otherwise, an additional (very quick) array scan could be made in the firmware with little effort.
If it is paramount to keep the same criterion for inputs and outputs (which is admittedly desirable, although it has been peacefully different until now), it could be debated that in a typical installations there are probably more input devices than output, therefore it might make sense to change the outputs instead.
This way the advantage of better traceability would be extended also to output messages.

@elral
Copy link
Collaborator Author

elral commented Oct 6, 2022

I am still not convinced that the pin number should be used.
The indentifier (x.te device) seems for me more straight forward. it is the same as for the outputs (see below). Also it is clearly expressed what the functionality behind is. I also try to spend as less memory as possible (I think as always known), but not in every case if understandibility gets reduced. Using an identifier does not mean additonal memory is required, it is the memory usage of the existing point to the name. The complete nameBuffer gets freed up.
For debugging the debug printouts can be enabled and an additional information about the pin number can be printed.

Also I would not change the output devices (one exception, see below). If changed an array of a given size for transforming is required which must be precessed. The array could be a fixed size (how big?) or must be also dynamically intialized (into the device buffer?). This would mean additional effort.

There is one exception I am wondering since month. The (LED) outputs are set directly by it's pin number and not via the "device ID". I havn't found a reason for this, and for a change the connector has to be changed and it is not anymore backwards compatible. Therefore I hadn't argued to change this. But now it would be a good opportunity to harmonize it with the other output devices.

@GioCC
Copy link
Contributor

GioCC commented Oct 7, 2022

Pardon my bluntness, but I fail to see any actual advantages mentioned for any of your objections.

Some of them seem to be on the lines of "seems better", but exactly why?
Or, "why bother considering improvements, we can make it like the other devices": but it's unclear exactly how these should be better.
Other points even seem to assume the opposite outcome of what I claimed (why "understandibility gets reduced"?).

The only actual point would regard the array to be added for output devices, which however I did not propose; in fact, as I mentioned, a very quick search would do the job. No array at all would be required.
BTW, the fact of saving a few more bytes is not a paramount goal, it's a free collateral consequence.

I can only point out one more time that the solution I propose does not come at the cost of any additional effort; we would be doing the same kind of work anyway.

@elral
Copy link
Collaborator Author

elral commented Oct 20, 2022

During the last days a played around with a Nano (which is a small Uno) and figured out, that we are really really short on RAM. Only "less" additions could be done before the Nano showed a strange behaviour (crashes at different "locations" of the firmware). And deactivating not needed devices will not clear up sooo much RAM (what I already did during testing).
Therefore it would be really good to solve and implement this issue to get more RAM available if we want to implement custom devices also for the Uno (what I would prefer).

@elral
Copy link
Collaborator Author

elral commented Nov 8, 2022

As we have aggreed on sending the deviceID instead of the name two weeks ago I have prepared the required changes for all input devices except the input multiplexer. The config.cpp is also already adapted (incl. the required changes for the multiplexer).
I will upload my branch this evening, latest tomorrow.
@GioCC may I ask you to do the changes for the input multiplexer? I am not so familiar with the code and no hardware to test it. Either you prepare an own pull request (preferable) or send me the changes to include them into my pull request.
If you issue your own pull request, please add the remark that this issue will be closed. I will not include this as I think I might be issue my pull request first.

Or is there another way to do it in a better way or what you prefer?

Thanks and regards

Ralf

@elral elral assigned elral and GioCC Nov 8, 2022
@GioCC
Copy link
Contributor

GioCC commented Nov 8, 2022

@GioCC may I ask you to do the changes for the input multiplexer?

Yes, gladly.

Either you prepare an own pull request (preferable) or send me the changes to include them into my pull request.

I'd generally say it would be good to have one single PR to address the issue in its entirety, but it also makes sense to split the two since you have your part ready before mine. Let's proceed as you suggested.

GioCC added a commit to GioCC/MobiFlight-FirmwareSource that referenced this issue Nov 9, 2022
@elral elral linked a pull request Jan 5, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants