Get rid of fixed device numbers #253

elral · 2023-08-21T09:47:48Z

Description of changes

The number of devices of each type will be calculated before the config gets loaded from the EEPROM.
With this information the device buffer gets filled up with the defined devices.
The device index array isn't needed anymore, instead a pointer to the first device of a type is stored.

fixes #174

Notes:

How to handle a full device buffer. For now it can happen that the device buffer gets filled completly, but during loading the devices this is not considered.
As the device index arrays are not needed anymore, this should be a huge RAM saving. But the compiler does only report a small saving

github-actions · 2023-08-21T09:55:28Z

Firmware for this pull request:
Firmware.zip

GioCC · 2023-08-21T17:21:08Z

OK, I had to look back at ancient history to dust off some old discussions.
This seems to be a less intrusive solution to tackle the problem that I tried to solve more radically with the abandoned, albeit once working, PR #175 (which also fixed #174, and then some).
That one probably looked too intimidating, while this PR - focusing on the sole storage issue - is indeed attractive (and definitely smart) in its simplicity.
Unfortunately I'm not able to test it right now because I'm on holiday, but I'm definitely going to have a deeper look at the code.
One point that comes to mind (may be a drawback, or not so much) is that, since the Connector does no longer have the notion of a hard limit to enforce for the number of devices, it won't be able to "stop the user in his tracks" during configuration, but it'll have to wait until the end and see whether the FW reports if the total configuration fits.
Alternatively, the Connector could try and compute the actual space requirement in advance, but that would require an even heavier parametrization than current one (plus, more involved computations which BTW would have to depend on the actual target MCU).

GioCC · 2023-08-21T17:25:51Z

src/Config.cpp

+        switch (command) {
+        case kTypeButton:
+            numberButtons++;
+            copy_success = readEndCommandFromEEPROM(&addreeprom); // check EEPROM until end of name


This statement occurs identical for each and every "case"; maybe better put just one instance at the end

GioCC · 2023-08-21T17:29:13Z

src/Config.cpp

+    // go through the EEPROM and calculate the number of devices for each type
+    do // go through the EEPROM until it is NULL terminated
+    {
+        switch (command) {


The "switch" could probably be avoided by placing the counter in an array indexed with the command value (as long as the kTypeXxxxx values are reasonably not sparse.
The whole case could disappear in something like numberDevice[command]++;

The kTypeXxxxx values are not perfect for an array due to the deprecated devices, but it makes still sense.

The kTypeXxxxx values are not perfect for an array due to the deprecated devices, but it makes still sense.

Exactly; however, each "useless" element would only cost 1 byte of RAM, while the total flash gain could be more interesting (still to be verified).

elral · 2023-08-22T04:45:48Z

One point that comes to mind (may be a drawback, or not so much) is that, since the Connector does no longer have the notion of a hard limit to enforce for the number of devices, it won't be able to "stop the user in his tracks" during configuration, ...

Up to now it is also not perfectly done. We have assumptions what could fit into the device buffer, but never checked all combinations of possible devices if they will fit into the device buffer. Therefore we send the kStatus message to the connector if the device buffer gets exceeded.

At least this message must be kept, but I was also thinking about to calculate the remaining space in the device buffer on the connector side like you mentioned. Each device type has a fixed size which is required in the device buffer, the size of the device buffer could be part of the json file. One exception is the upcoming custom device. In this case the size is unknown which could lead to report the size within the json file (and due to this for all devices?)

I am still searching where the huge memory of all device index arrays is gone to. The proMicro has in sum 70 devices of 11 different types. The saving should be (70-11)*8=472bytes*, but the compiler reports only ~70bytes.

Thanks a lot for your comments and a nice vacation!

*each array entry for the device index arrays needs 8 bytes, for all devices one pointer of this size still required

GioCC · 2023-08-22T06:48:47Z

Just a vague idea: maybe the connector could send a message when adding (or removing) a device, during configuration (before saving it to the board), to which the board could return the required storage size increment/decrement. (Adding "no device" would return the occupation level for the currently saved configuration). Then the connector could show the updated prospect memory occupation (possibly in %).

I am still searching where the huge memory of all device index arrays is gone to. The proMicro has in sum 70 devices of 11 different types. The saving should be (70-11)8=472bytes, but the compiler reports only ~70bytes.

That is something I'm also curious to investigate. Have you already tried using the memory analysis ("Inspect") function of PlatformIO?

elral · 2023-08-22T07:29:59Z

That is something I'm also curious to investigate. Have you already tried using the memory analysis ("Inspect") function of PlatformIO?

I tried this, but unfortunately the inspection aborts due to a failure (have to check at home which one). I had this a couple of times also on other projects.

elral · 2023-08-22T07:34:22Z

Just a vague idea: maybe the connector could send a message when adding (or removing) a device, during configuration (before saving it to the board), to which the board could return the required storage size increment/decrement. (Adding "no device" would return the occupation level for the currently saved configuration). Then the connector could show the updated prospect memory occupation (possibly in %).

This could be a way. Since the same memory for devices is available it should be no problem for existing configurations (not to say it's not required). Maybe a worst case calculation with considering the available pins and the most consuming device might help

elral · 2023-08-23T06:33:56Z

I am still searching where the huge memory of all device index arrays is gone to. The proMicro has in sum 70 devices of 11 different types. The saving should be (70-11)8=472bytes, but the compiler reports only ~70bytes.

That is something I'm also curious to investigate. Have you already tried using the memory analysis ("Inspect") function of PlatformIO?

Made again the failure to trust hovering about sizeof(). Sometimes it reports wrong values. A pointer is 2 byte instead of 8byte. So the formula must be (70-11)*2bytes = 118 bytes which should be saved. Compared this to 76bytes saving makes much more sense. 42 bytes less saving for the new functions seems reasonable...

GioCC · 2023-08-23T09:49:56Z

src/config.h

@@ -23,6 +23,8 @@ enum {
    kTypeMuxDriver,           // 13 Multiplexer selector support (generates select outputs)
    kTypeDigInMux,            // 14 Digital input multiplexer support (example: 74HCT4067, 74HCT4051)
    kTypeStepper              // new stepper type with settings for backlash and deactivate output
+    // if new device types are added, change 'kTypeStepper' for the array numberDevices[] in readConfig() to the new one!!


Suggestion: add a last entry like this (comments removed for clarity)

// ... kTypeDigInMux, kTypeStepper, // Add new devices here, kTypeMAXCOUNT

and use that value for the array size in getArraysizes(). This way no manual update will be required when adding devices.

Again a very good idea!

GioCC · 2023-08-23T10:13:32Z

Made again the failure to trust hovering about sizeof(). Sometimes it reports wrong values. A pointer is 2 byte instead of 8byte.

Yes, that's an important thing to know.
The indications given by IntelliSense in VSCode are based upon the compiler settings configured in c_cpp_properties.json (see https://code.visualstudio.com/docs/cpp/configure-intellisense-crosscompilation for reference).
More than one option can be configured; in this case they will appear in the info bar on the bottom right.
The catch is that, though apparently it should be possible to select e.g. the gcc-avr compiler (see "Compiler path" section), that doesn't work: the only options available are the architectures selectable with "IntelliSense mode", and these are all 32- or 64- bit.
I haven't yet found a way to extend this list (and I believe it isn't possible).

During development, whenever possible, I usually write a section of debug code like this:

void report(void)
{
    sprintf(sbuf, "Size of %s: %d bytes\n\r", "MyClass1", sizeof(MyClass1) );   Serial.println(sbuf);
    sprintf(sbuf, "Size of %s: %d bytes\n\r", "MyType2", sizeof(MyType2) );   Serial.println(sbuf);
    //...
}

so as to have the actual running code report the relevant sizes once and for all.

github-actions · 2023-08-23T13:02:59Z

Firmware for this pull request:
Firmware.zip

elral · 2023-09-05T09:44:43Z

According the below calculations for the device buffer w/o fixed device numbers the limitations are coming from the max. pins which are availabe (except for the Nano/Uno) if the device buffer will be increased for the Mega and ProMicro. For the Mega it's no problem, he has plenty of available RAM. For the ProMicro we could benefit from the RAM saving (with spending additional 3 bytes).
The Nano/Uno is some kind of problematic. Increasing the max. number of analog inputs from 6 to 8 via PR 248 wasn't a really good idea. I would roll back again to max. 6 analog inputs which saves 58 bytes. Additionally I would limit the max. numbers of servos to 6 (for now it is 2) which would save 92 bytes. For the missing 10 bytes we can profite from the RAM saving.

This would mean that we stay with the definition of max. devices within each board.json file. It is also still sensefull to limit the max. input (and output?) devices per type to considere the processing time. In this case an online check of the RAM usage of the device buffer is not required, either on the connector side nor on the FW side.

Or we are just using the existing message, that the device buffer is filled and no more device could be added. This will only happpen for the Nano/Uno. Here also an online check of the RAM usage of the device buffer is not required, either on the connector side nor on the FW side. "Problems" with the processing time due to too much input (output?) devices would be in the responsibility of the user. Hints who many devices per type should not be exceeded should be done. In this case no limitation in the board.json files are required.

However, more than these changes would not be required as no communication with the connector is required (except the existing warning which might be required for the Nano/Uno).

Required device memory for AVR's

	AVR's	Pico
analog device	29 byte	40 byte
button	4 byte	8 byte
digital input MUX	6 byte	8 byte
encoder	31 byte	36 byte
input shifter	11 byte	16 byte
lcdDisplay	18 byte	24 byte
output	2 byte	2 byte
Output Shifter	9 byte	9 byte
Led Segment	25 byte	36 byte
Servos	23 byte	64 byte
Stepper	82 byte	100 byte

Required bytes per pin per device

	pins	byte/pin AVR's	byte/pin Pico
analog device	1	29 byte	40 byte
Servos	1	23 byte	64 byte
Stepper	4	20.5 byte	25 byte
encoder	2	15.5 byte	18 byte
lcdDisplay	2*	9 byte	12 byte
Led Segment	3	8.3 byte	12 byte
digital input MUX	5	6 byte	1.2 byte
button	1	4 byte	4 byte
input shifter	3	3.6 byte	5.3 byte
Output Shifter	3	3 byte	3 byte
output	1	2 byte	2 byte

for multiple LCD's only 2 pins are required

Mega:
Size of device buffer is: 1500 bytes
w/o fixed device numbers RAM saving is 289 bytes
number of pins: 68
number of analog pins: 16

required bytes in device buffer actually:

16 * analog -> 464 bytes
10 * servo -> 230 bytes
10 * stepper -> 820 bytes
1 * encoder -> 31 bytes
max bytes -> 1545 bytes -> exceding device buffer by 45 bytes

required bytes in device buffer w/o fixed device numbers:

16 * analog -> 464 bytes
48 * servo -> 1104 bytes
1 * stepper -> 21 bytes
max bytes -> 1589 bytes -> exceding device buffer, can be increased w/o any impact

Nano/Uno:
Size of device buffer is: 300 bytes
w/o fixed device numbers RAM saving is 55 bytes
number of pins: 18
number of analog pins: 8

required bytes in device buffer actually:

8 * analog -> 232 bytes
2 * servo -> 46 bytes
2 * stepper -> 164 bytes
max bytes -> 442 bytes -> exceding device buffer by 142 bytes!!

required bytes in device buffer w/o fixed device numbers:

8 * analog -> 232 bytes
10 * servo -> 230 bytes
max bytes -> 460 bytes -> exceding device buffer by 160 bytes!!

ProMicro:
Size of device buffer is: 400 bytes
w/o fixed device numbers RAM saving is 65 bytes
number of pins: 18
number of analog pins: 9

required bytes in device buffer actually:

9 * analog -> 261 bytes
3 * servo -> 69 bytes
1 * stepper -> 82 bytes
1 * encoder -> 31 bytes
max bytes -> 443 bytes -> exceding device buffer by 43 bytes!!

required bytes in device buffer w/o fixed device numbers:

9 * analog -> 261 bytes
9 * servo -> 207 bytes
max bytes -> 468 bytes -> exceding device buffer by 68 bytes!!

Raspberry Pico:
Size of device buffer is: 2000 bytes
w/o fixed device numbers RAM saving is 324 bytes
number of pins: 26
number of analog pins: 3

required bytes in device buffer actually, does not change for w&o fixed device numbers:

3 * analog -> 120 bytes
8 * servo -> 512 bytes
3 * stepper -> 300 bytes
1 * encoder -> 36 bytes
1 * button -> 8 bytes
max bytes -> 976 bytes -> device buffer could be reduced.

Limitations for max. devices are

max. available pins
max. available analog pins
processing time for inputs (each loop these are processed) -> most important
- encoder
- input shifter
- input multiplexer
processing time for outputs (only for new values processed)
- LCD's
- output shifter
- 7 segment displays
- stepper

	Atmega	Pico	Remark
Outputs	11 us		only writing one output, analogWrite(), no changes
Buttons	9 us	2 us	only reading one input
	224 us	220 us (??)	reading AND evaluating one input, must be wrong for the Pico
LEDSegment	6.3 ms	1.3 ms	2x8 daisy chained
Encoder	23 us	3 us	only reading one encoder
	290 us	22 us	reading AND evaluating one
Stepper	23 ms	3 us	not changed to FastIO
Servos
LCD	12 ms	9.7 ms	limited by I2C
AnalogIn	122 us	11 us	calculating average value
OutputShifter	0.74 ms		4x8bit daisy chained
InputShifter	0.5 ms		4x8bit daisy chained
Multiplexer	2.4 ms		4 x 16 bit

This reverts commit 27abcd9.

github-actions · 2023-10-25T15:30:46Z

Firmware for this pull request:
Firmware.zip

github-actions · 2023-10-30T15:35:25Z

Firmware for this pull request:
Firmware.zip

This reverts commit 974b9a2.

elral added 12 commits August 19, 2023 17:20

dynamical button numbers

03c36c8

set up array function

45edfaf

better comments

ce9d55c

all devices with dymically defined array

701dcc1

changes for MUX

088167d

missing defines added

c080f58

missing return statement

34e83fc

back to original device memory space

1762966

debug print out added

03ec932

extended information of memory usage to serial, deactivated

97ed2d6

clean up, delete max. number of devices from board file

838888e

get servos to run

f77ebe8

GioCC reviewed Aug 21, 2023

View reviewed changes

elral self-assigned this Aug 22, 2023

elral added 3 commits August 23, 2023 08:54

add comment for array size

dcbb30a

get array size via an array

cb038b2

error handling added

f2b0955

GioCC reviewed Aug 23, 2023

View reviewed changes

better definition of array size

666fec8

elral requested a review from DocMoebiuz as a code owner September 8, 2023 17:12

elral added 14 commits October 11, 2023 16:20

Merge branch 'MobiFlight:main' into no_fixed_device_numbers

8584aaa

max. numbers of devices not required anymore

517385b

one common function for sending failure message

63f9c36

failure message directly from flash

8dd5291

better naming

096be82

delete not required parameter in handler functions

27abcd9

Revert "delete not required parameter in handler functions"

0db4409

This reverts commit 27abcd9.

Merge branch 'main' into no_fixed_device_numbers

50a7526

merge branch main

018b451

delete not required folder

4e8eba1

Merge branch 'main' into no_fixed_device_numbers

8cb5d3b

Merge branch 'main' into no_fixed_device_numbers

d2a42d5

merge branch main

daac96f

also custom devices w/o device limit

64403aa

setup array for custom device was missing

910f7b2

DocMoebiuz approved these changes Nov 4, 2023

View reviewed changes

DocMoebiuz merged commit 974b9a2 into MobiFlight:main Nov 4, 2023
1 check passed

elral deleted the no_fixed_device_numbers branch November 4, 2023 08:49

This was referenced Nov 5, 2023

New 7 segment type gets not loaded #266

Closed

Beta - New 7 segment type gets loaded correctly #267

Merged

DocMoebiuz added the feature label Nov 6, 2023

elral added a commit to elral/MobiFlight-FirmwareSource that referenced this pull request Nov 7, 2023

Revert "Get rid of fixed device numbers (MobiFlight#253)"

5eb5d70

This reverts commit 974b9a2.

elral mentioned this pull request Nov 8, 2023

Beta - Fix memory overlap in DeviceBuffer #272

Merged

DocMoebiuz mentioned this pull request Nov 19, 2023

Allow more than 4 registers for 74HC165 and 74HC595 MobiFlight/MobiFlight-Connector#1323

Open

This was referenced Nov 22, 2023

Pin 1 gets set as output or input when a button/analogIn is defined #284

Closed

init devices with attach() function and not via constructor #286

Merged

elral mentioned this pull request Dec 15, 2023

Increase max. devices per type MobiFlight/MobiFlight-Connector#1502

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of fixed device numbers #253

Get rid of fixed device numbers #253

elral commented Aug 21, 2023 •

edited by DocMoebiuz

Loading

github-actions bot commented Aug 21, 2023

GioCC commented Aug 21, 2023 •

edited

Loading

GioCC Aug 21, 2023

GioCC Aug 21, 2023

elral Aug 22, 2023

GioCC Aug 22, 2023

elral commented Aug 22, 2023

GioCC commented Aug 22, 2023

elral commented Aug 22, 2023

elral commented Aug 22, 2023

elral commented Aug 23, 2023 •

edited

Loading

GioCC Aug 23, 2023 •

edited

Loading

elral Aug 23, 2023

GioCC commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023

elral commented Sep 5, 2023

github-actions bot commented Oct 25, 2023

github-actions bot commented Oct 30, 2023

Get rid of fixed device numbers #253

Get rid of fixed device numbers #253

Conversation

elral commented Aug 21, 2023 • edited by DocMoebiuz Loading

Description of changes

github-actions bot commented Aug 21, 2023

GioCC commented Aug 21, 2023 • edited Loading

GioCC Aug 21, 2023

Choose a reason for hiding this comment

GioCC Aug 21, 2023

Choose a reason for hiding this comment

elral Aug 22, 2023

Choose a reason for hiding this comment

GioCC Aug 22, 2023

Choose a reason for hiding this comment

elral commented Aug 22, 2023

GioCC commented Aug 22, 2023

elral commented Aug 22, 2023

elral commented Aug 22, 2023

elral commented Aug 23, 2023 • edited Loading

GioCC Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

elral Aug 23, 2023

Choose a reason for hiding this comment

GioCC commented Aug 23, 2023 • edited Loading

github-actions bot commented Aug 23, 2023

elral commented Sep 5, 2023

github-actions bot commented Oct 25, 2023

github-actions bot commented Oct 30, 2023

elral commented Aug 21, 2023 •

edited by DocMoebiuz

Loading

GioCC commented Aug 21, 2023 •

edited

Loading

elral commented Aug 23, 2023 •

edited

Loading

GioCC Aug 23, 2023 •

edited

Loading

GioCC commented Aug 23, 2023 •

edited

Loading