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

isr_tables: Simplify how the spurious irq function address is found #7574

Merged

Conversation

SebastianBoe
Copy link
Collaborator

@SebastianBoe SebastianBoe commented May 16, 2018

Instead of finding the address of the spurious irq function in the
intList section we now rely on the linker to find the address in the
_irq_spurious symbol.

This is a migration from using code generation to using the C language
which we in the general case should aways strive towards.

In this specific case it makes the generated code 'irq_tables.c'
easier to read as we replace magic numbers with the &_irq_spurious
token.

Also, the path through the build system that _irq_spurious makes is
much shorter, so it is much easier for a user to understand how it is
used.

Signed-off-by: Sebastian Bøe sebastian.boe@nordicsemi.no

@SebastianBoe SebastianBoe added Enhancement Changes/Updates/Additions to existing features area: Build System labels May 16, 2018
@SebastianBoe
Copy link
Collaborator Author

SebastianBoe commented May 16, 2018

In general, I'm trying to simplify the build system by limiting the the code-generation as much as possible and doing as much as possible just using the C language.

Will also look into getting out of pushing sw_irq_handler through a code generator.

@codecov-io
Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #7574 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7574   +/-   ##
=======================================
  Coverage   55.01%   55.01%           
=======================================
  Files         483      483           
  Lines       53949    53949           
  Branches    10494    10494           
=======================================
  Hits        29680    29680           
  Misses      19983    19983           
  Partials     4286     4286

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c7a44b...e863495. Read the comment docs.

@SebastianBoe
Copy link
Collaborator Author

I also have a patch for sw_irq_handler, I can do that in a separate or PR or this one depending on the reviewers preference.

SebastianBoe@b4bd701

@@ -209,6 +206,8 @@ def main():
prefix = endian_prefix()
numisrs = intlist["num_isrs"]

spurious_handler = "&_irq_spurious"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hides reference to _irq_spurious making code harder to read.
In my opinion we should put a comment near definition of _irq_spurious stating something like that:
The name of the symbol cannot be changed as it is referenced by generated code. For more information look into gen_isr_tables.py.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is problematic. But I have a different solution proposal.

What I am actually doing here, is trying to find/rework/create some mechanism that does
code generation according to best-practices.

When I have that I can use @b0661's codegen infrastructure here to prove that codegen will be a useful tool to support our code-generation use-cases.

With codegen it will eventually look something like this:

/* Not AUTO-GENERATED by gen_isr_tables.py, feel free to edit! */

#include <toolchain.h>
#include <linker/sections.h>
#include <sw_isr_table.h>
#include <arch/cpu.h>

// Referenced by irq_vector_table.h
#if defined(CONFIG_GEN_SW_ISR_TABLE) && defined(CONFIG_GEN_IRQ_VECTOR_TABLE)
#define ISR_WRAPPER ((u32_t)&_isr_wrapper)
#else
#define ISR_WRAPPER NULL
#endif

// Referenced by sw_isr_table.h
#define IRQ_SPURIOUS (&_irq_spurious)

u32_t __irq_vector_table _irq_vector_table[26] = {
    #include <generated/irq_vector_table.h>
};

struct _isr_table_entry __sw_isr_table _sw_isr_table[26] = {
    #include <generated/sw_isr_table.h>
};

The above code will not be generated, so the user will easily be able to understand that _irq_spurious is used by sw_isr_table.h.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version places _irq_spurious into the table as a placeholder. This will make it clearer how _irq_spuirous is used (and fix a bug).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indeed looks better.

Instead of finding the address of the spurious irq function in the
intList section we now rely on the linker to find the address in the
_irq_spurious symbol.

This is a migration from using code generation to using the C language
which we in the general case we should aways strive towards.

In this specific case it makes the generated code 'irq_tables.c'
easier to read as we replace magic numbers with the &_irq_spurious
token.

Also, the path through the build system that _irq_spurious makes is
much shorter, so it is much easier for a user to understand how it is
used.

Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>
@SebastianBoe
Copy link
Collaborator Author

@andrewboie , @nashif : This PR needs review.

@andrewboie andrewboie merged commit 8f321b4 into zephyrproject-rtos:master Jun 19, 2018
SebastianBoe added a commit to SebastianBoe/zephyr that referenced this pull request Jun 20, 2018
This is a migration from using code generation to using the C language
which we in the general case we should aways strive towards.

It is equivalent to the simplification that was done with
_irq_spurious here:
zephyrproject-rtos#7574

Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>
nashif pushed a commit that referenced this pull request Jun 20, 2018
This is a migration from using code generation to using the C language
which we in the general case we should aways strive towards.

It is equivalent to the simplification that was done with
_irq_spurious here:
#7574

Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants