chipKIT32 / chipKIT-core

Downloadable chipKIT core for use with Arduino 1.6 - 1.8+ IDE, PlatformIO, and UECIDE
http://chipkit.net/
Apache License 2.0
59 stars 53 forks source link

Ability to have more than two additional core timer interrupts #452

Open jpswensen opened 5 years ago

jpswensen commented 5 years ago

We have run into an issue lately where we wanted to have more than two additional core timer interrupts. Now, I realize we could munge together multiple interrupts and makes sure they execute at integer multiples of each other, but it was just as easy to go into wiring.c on line 481 and add more entries into the gCoreTimerInfo[] array.

I looked through the way that the timer interrupts are handles and it seems that there would be little to no execution overhead to adding in 3-4 more slots in gCoreTimerInfo[], and only 8 bytes or so in terms of RAM (I'm not exactly sure the size of a function pointer on this architecture/compiler).

Option 1: So, I guess my question is whether you would be averse to a pull request that adds 3 more slots in the gCoreTimerInfo[] array, allowing a total of 5 timer interrupts based on the Core Timer?

Options 2: The other option I was thinking about was have a #define that a user could #undef that wrapped the gCoreTimerInfo[] declaration and let a user externally define their own array. Thoughts on this one?

Options 3: I just live with it and go modify wiring.c on ever machine we use to build this chipkit code. This is our fallback, but there is no compile time or run time information that someone is trying to attach too many timer interrupts. It just fails to do it.

majenkotech commented 5 years ago

Option 4:

Re-factor the code to use a linked list, so the only limit is the amount of free heap space.

JacobChrist commented 5 years ago

I like option 2 as an intern solution and maybe option 4 as a long term solution. My only comment on option 4 would be that unless there is a need for lots of timers the linked list overhead may be greater for more people that for the few it benefits.

Jacob

On Tue, Apr 23, 2019, 9:26 AM John Swensen notifications@github.com wrote:

We have run into an issue lately where we wanted to have more than two additional core timer interrupts. Now, I realize we could munge together multiple interrupts and makes sure they execute at integer multiples of each other, but it was just as easy to go into wiring.c on line 481 and add more entries into the gCoreTimerInfo[] array.

I looked through the way that the timer interrupts are handles and it seems that there would be little to no execution overhead to adding in 3-4 more slots in gCoreTimerInfo[], and only 8 bytes or so in terms of RAM (I'm not exactly sure the size of a function pointer on this architecture/compiler).

Option 1: So, I guess my question is whether you would be averse to a pull request that adds 3 more slots in the gCoreTimerInfo[] array, allowing a total of 5 timer interrupts based on the Core Timer?

Options 2: The other option I was thinking about was have a #define that a user could

undef that wrapped the gCoreTimerInfo[] declaration and let a user

externally define their own array. Thoughts on this one?

Options 3: I just live with it and go modify wiring.c on ever machine we use to build this chipkit code. This is our fallback, but there is no compile time or run time information that someone is trying to attach too many timer interrupts. It just fails to do it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM26VMZIH24MPUPSRG4LPR42FBANCNFSM4HH3BZYQ .

majenkotech commented 5 years ago

Processing overhead, or memory overhead? There's little-to-no processing overhead for a linked list. Instead of incrementing a value and using it as an index to a table in memory you're just assigning a pointer to a variable. If anything it's faster.

for (int i = 0; i < 4; i++) {
    (do maths here to get the address that i should relate to, then do something with it)
}

for (struct foo *f = head; f; f = f->next) {
    (do something with f->...)
}
JacobChrist commented 5 years ago

Memory. Its also probably so insignificant that its not worth worrying about. I guess if your not using any interrupts then there may be a gain since your only need to store a kill longer.

Jacob

On Tue, Apr 23, 2019, 11:41 AM Majenko Technologies < notifications@github.com> wrote:

Processing overhead, or memory overhead? There's little-to-no processing overhead for a linked list. Instead of incrementing a value and using it as an index to a table in memory you're just assigning a pointer to a variable. If anything it's faster.

for (int i = 0; i < 4; i++) { (do maths here to get the address that i should relate to, then do something with it) }

for (struct foo *f = head; f; f = f->next) { (do something with f->...) }

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485925872, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM22QYNHUFKUMBOYKEBTPR5J7JANCNFSM4HH3BZYQ .

JacobChrist commented 5 years ago

Null pointer not kill pointer. Ay Google.

On Tue, Apr 23, 2019, 11:50 AM Jacob Christ jacob@pontech.com wrote:

Memory. Its also probably so insignificant that its not worth worrying about. I guess if your not using any interrupts then there may be a gain since your only need to store a kill longer.

Jacob

On Tue, Apr 23, 2019, 11:41 AM Majenko Technologies < notifications@github.com> wrote:

Processing overhead, or memory overhead? There's little-to-no processing overhead for a linked list. Instead of incrementing a value and using it as an index to a table in memory you're just assigning a pointer to a variable. If anything it's faster.

for (int i = 0; i < 4; i++) { (do maths here to get the address that i should relate to, then do something with it) }

for (struct foo *f = head; f; f = f->next) { (do something with f->...) }

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485925872, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM22QYNHUFKUMBOYKEBTPR5J7JANCNFSM4HH3BZYQ .

jpswensen commented 5 years ago

I guess I am a little uncomfortable with doing memory allocation for something so simple. A quick search through the rest of chipKIT core shows that very little usage of malloc and new are used and I am reticent to use it for something like this.

On Apr 23, 2019, at 11:41 AM, Majenko Technologies notifications@github.com wrote:

Processing overhead, or memory overhead? There's little-to-no processing overhead for a linked list. Instead of incrementing a value and using it as an index to a table in memory you're just assigning a pointer to a variable. If anything it's faster.

for (int i = 0; i < 4; i++) { (do maths here to get the address that i should relate to, then do something with it) }

for (struct foo *f = head; f; f = f->next) { (do something with f->...) } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485925872, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFQKJFZEJULAPWRRKGE3ADPR5J7LANCNFSM4HH3BZYQ.

JacobChrist commented 5 years ago

I think malloc is fine as long as you never free due to lack of garbage collection. But this is just an option and I don't really know how malloc and free are working under the good.

Jacob

On Tue, Apr 23, 2019, 11:51 AM John Swensen notifications@github.com wrote:

I guess I am a little uncomfortable with doing memory allocation for something so simple. A quick search through the rest of chipKIT core shows that very little usage of malloc and new are used and I am reticent to use it for something like this.

On Apr 23, 2019, at 11:41 AM, Majenko Technologies < notifications@github.com> wrote:

Processing overhead, or memory overhead? There's little-to-no processing overhead for a linked list. Instead of incrementing a value and using it as an index to a table in memory you're just assigning a pointer to a variable. If anything it's faster.

for (int i = 0; i < 4; i++) { (do maths here to get the address that i should relate to, then do something with it) }

for (struct foo *f = head; f; f = f->next) { (do something with f->...) } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485925872>, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAFQKJFZEJULAPWRRKGE3ADPR5J7LANCNFSM4HH3BZYQ .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485929395, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM2ZNAO2E36P2SGMAUOLPR5LDTANCNFSM4HH3BZYQ .

majenkotech commented 5 years ago

Memory would be just 4 bytes per entry. However heap memory is at a premium thanks to Microchip's crippled memory handling - allocating a fixed amount to the heap memory, something which I have been campaigning for years and years to have lifted.

In general malloc/new etc are frowned upon in microcontrollers, mainly because they have traditionally had only very small amounts of memory. However we generally have lots and lots, and if it weren't for the limit (just 2k in most linker scripts IIRC) then using new/malloc etc would be far more common.

majenkotech commented 5 years ago

Garbage collection is irrelevant. That's only for systems that don't require manual free/delete. The problem with heap is fragmentation, and that is only a problem when you have a tiny limit, like on an AVR.

If you are allocating at the start of your program and never freeing, as would probably be the case with these interrupts, then there is no fragmentation.

The biggest culprit is String, and that is generally down to people not knowing how to use it properly.

JacobChrist commented 5 years ago

What about free?

On Tue, Apr 23, 2019, 11:54 AM Majenko Technologies < notifications@github.com> wrote:

Memory would be just 4 bytes per entry. However heap memory is at a premium thanks to Microchip's crippled memory handling - allocating a fixed amount to the heap memory, something which I have been campaigning for years and years to have lifted.

In general malloc/new etc are frowned upon in microcontrollers, mainly because they have traditionally had only very small amounts of memory. However we generally have lots and lots, and if it weren't for the limit (just 2k in most linker scripts IIRC) then using new/malloc etc would be far more common.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485930291, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM2ZYDJEW247ITZ6DPGDPR5LM5ANCNFSM4HH3BZYQ .

majenkotech commented 5 years ago

What about it?

JacobChrist commented 5 years ago

You already addressed it and I agree with your assessment that fragmentation being the issue.

On Tue, Apr 23, 2019, 11:57 AM Majenko Technologies < notifications@github.com> wrote:

What about it?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485931411, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM22B6VFS7KR7MWRGAGLPR5LYXANCNFSM4HH3BZYQ .

majenkotech commented 5 years ago

Fragmentation in this scenario will probably not exist. Or if it does it will be so minor as to not even be noticeable. After all, how often will you add and remove core timer services?

JacobChrist commented 5 years ago

Yes agreed. This was my initial thought, Google mind reading just doesn't work yet [thank goodness because then you would know how I really feel about this topic]. ;-)

On Tue, Apr 23, 2019, 12:00 PM Majenko Technologies < notifications@github.com> wrote:

Fragmentation in this scenario will probably not exist. Or if it does it will be so minor as to not even be noticeable. After all, how often will you add and remove core timer services?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485932662, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM26VANKKSKBOWY4EIX3PR5MGBANCNFSM4HH3BZYQ .

majenkotech commented 5 years ago

Well, I just tried re-implementing the CT service with a linked list, and it all crashed and burned. It'll be a longer term job this one, so I guess for now either we just increase the default size of the array, or do something fancy with #defines - though not sure what off hand.

JacobChrist commented 5 years ago

Sounds like a good plan. ;-)

On Tue, Apr 23, 2019, 12:50 PM Majenko Technologies < notifications@github.com> wrote:

Well, I just tried re-implementing the CT service with a linked list, and it all crashed and burned. It'll be a longer term job this one, so I guess for now either we just increase the default size of the array, or do something fancy with '#defines - though not sure what off hand.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/452#issuecomment-485948714, or mute the thread https://github.com/notifications/unsubscribe-auth/AABQM24PUQ5KP32J5LHVWJLPR5R6LANCNFSM4HH3BZYQ .