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

Change all function prefix from up_ to arch_ in include/nuttx/arch.h #915

Closed
xiaoxiang781216 opened this issue Apr 30, 2020 · 13 comments
Closed
Assignees

Comments

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Apr 30, 2020

@patacongo does this make sense?

@xiaoxiang781216 xiaoxiang781216 self-assigned this Apr 30, 2020
@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Apr 30, 2020

The best step is:
1.Rename functions in arch.h from up_xxx to arch_xxx
2.Rename files in arch/yyy/ from up_xxx.* to yyy_xxx.*
3.Rename function in arch/yyy/ from up_xxx to yyy_xxx
We can avoid the error like this:
#916
And the global find and replace should work correctly.

@patacongo
Copy link
Contributor

The naming convenions required that global function platform fucntions .. that is, functions used outside of arch/ and board/ MUST begion with up. Functions used only within arch/ and board/ must NOT begin with up_. See https://cwiki.apache.org/confluence/display/NUTTX/Naming+FAQ

arch_ is never permitted as a function prefix.

@patacongo
Copy link
Contributor

patacongo commented Apr 30, 2020

In #916, the error is in the name up_fullcontextrestore(tcb->xcp.regs); which is incorrect. That should be arm_fullcontextrestore(). The naming in other ARM architectures is incorrect. The other architectures are not following the naming standard. ARMv8-M is correctly following the naming standard.

So yes, there will be issues UNTIL other ARM architectures are mad to follow the naming standard. Let's do that!! Let's fix the other ARM architectures and not screw up the naming standard.

@xiaoxiang781216
Copy link
Contributor Author

The naming convenions required that global function platform fucntions .. that is, functions used outside of arch/ and board/ MUST begion with up. Functions used only within arch/ and board/ must NOT begin with up_. See https://cwiki.apache.org/confluence/display/NUTTX/Naming+FAQ

arch_ is never permitted as a function prefix.

As you state in https://cwiki.apache.org/confluence/display/NUTTX/Naming+FAQ:
Q: What's the meaning of the prefix up_ in NuttX? Which functions should be prefixed with up_?
A: up_ is supposed to stand for microprocessor; the u is like the Greek letter micron: µ. So it would be µP which is a common shortening of the word microprocessor. I don't like that name very much. I wish I would have used arch_ instead. But now I think I am stuck with up_.
Aslo there are some functions start with arch_ in arch.h now:
int arch_phy_irq(FAR const char *intf, xcpt_t handler, void *arg,
phy_enable_t *enable);
void arch_sporadic_start(FAR struct tcb_s *tcb);
void arch_sporadic_lowpriority(FAR struct tcb_s *tcb);
void arch_sporadic_suspend(FAR struct tcb_s *tcb);
void arch_sporadic_resume(FAR struct tcb_s *tcb);

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Apr 30, 2020

In #916, the error is in the name up_fullcontextrestore(tcb->xcp.regs); which is incorrect. That should be arm_fullcontextrestore(). The naming in other ARM architectures is incorrect. The other architectures are not following the naming standard. ARMv8-M is correctly following the naming standard.

So yes, there will be issues UNTIL other ARM architectures are mad to follow the naming standard. Let's do that!! Let's fix the other ARM architectures and not screw up the naming standard.

We just want to change up_xxx function common across armv7-a, armv7-m... to arm_xxx, but up_ is also used for the common function across arm, riscv..., so it is impossible to replace all up_ inside arch/arm to arm_ auotmaitcally.
if https://cwiki.apache.org/confluence/display/NUTTX/Naming+FAQ state correctly, arch_ is better prefix than up_. We can apply the sequence I suggested before:
1.Rename functions in arch.h from up_xxx to arch_xxx
2.Rename files in arch/yyy/ from up_xxx.* to yyy_xxx.*
3.Rename function in arch/yyy/ from up_xxx to yyy_xxx
Since functions for all arch change to arch_ in the first step, we can find the remaining up_xxx and replace with arm_xxx safely and automatically.

@patacongo
Copy link
Contributor

I have started the renaming with PR #924 After that is merged I will do more, a little at a time.

@patacongo
Copy link
Contributor

We just want to change up_xxx function common across armv7-a, armv7-m... to arm_xxx, but up_ is also used for the common function across arm, riscv..., so it is impossible to replace all up_ inside arch/arm to arm_ auotmaitcally.

Common functions across RISC-V (only) should begin with riscv_, not up and not up_ or arch_

There is not interaction between the private functions in ARM and the private functions in other archtectures such as RISC-V. That is proper modular design. The naming must all be private and internale to each architecture and should NOT be shared.

@patacongo
Copy link
Contributor

Private functions used only within arch/ and board/ for the same MCU should never have the prefix up_ (or arch_) and should never be exposed via arch.h. That is very bad modular thinking and we must not even consider that.

up_ is intended for common MCU interfaces that are used by the OS, usually in sched/ and must always have the prefix up_ and must always be prototypes in include/nuttx/arch.h. I disagree with you completely and think that this is a terrible idea. It is not good modular thinking and we must not consider it.

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Apr 30, 2020

We just want to change up_xxx function common across armv7-a, armv7-m... to arm_xxx, but up_ is also used for the common function across arm, riscv..., so it is impossible to replace all up_ inside arch/arm to arm_ auotmaitcally.

Common functions across RISC-V (only) should begin with riscv_, not up and not up_ or arch_

There is not interaction between the private functions in ARM and the private functions in other archtectures such as RISC-V. That is proper modular design. The naming must all be private and internale to each architecture and should NOT be shared.

Private functions used only within arch/ and board/ for the same MCU should never have the prefix up_ (or arch_) and should never be exposed via arch.h. That is very bad modular thinking and we must not even consider that.

up_ is intended for common MCU interfaces that are used by the OS, usually in sched/ and must always have the prefix up_ and must always be prototypes in include/nuttx/arch.h. I disagree with you completely and think that this is a terrible idea. It is not good modular thinking and we must not consider it.

You misunderstand my meaning, let me give an example:
1.Find and replace all up_idle to arch_idle
2.Repeat step 1 for all functions inside include/nuttx/arch.h
3.Find and replace all up_doirq to arm_doirq under arch/arm
4.Repeat step 3 for all up_ function under arch/arm
5.Change up_xxx function under arch/riscv to riscv_xxx
The final result is what you want:
1.The code outside arch call arch_xxx
2.The common function for arm prefix with arm_
3.The common fucntion for riscv prefix with rsicv_
But this approach we can do the find/replace automatically.
The key point is that include/nuttx/arch.h need take a new prefix arch_, then we know that it's safe to change the remaining up_ functions in each arch folder to the corrospending prefix(e.g. arm_ or riscv_).

@patacongo
Copy link
Contributor

@xiaoxiang781216 I have put a lot of effort in the past few days and submitted several PRs to bring the arch/arm directories into compliance with the naming convention of https://cwiki.apache.org/confluence/display/NUTTX/Naming+FAQ

I think if we follow the naming convention, we should not run into any future issues like #916

@xiaoxiang781216
Copy link
Contributor Author

Sure, let's clean up arm arch first, and see whether change up_ to arch_ for all function inside arch.h can help rename other arch more efficiently.

@patacongo
Copy link
Contributor

I just want to make sure that we protect the modular architecture. I don't want to expose ARM internal functions though arch.h. That would not be a good decision. Just changing the naming from up_ to arch_ is not so important (but effects a lot of code, lots of documentation, and the Wiki).

It is probably better to stay wtih the prefix up_ for historic reasons.

@patacongo
Copy link
Contributor

The internal architecture interfaces are not documented and, hence, have no impact outside of the architecture-specific code and the architecture-specific documentation.

The up_ functions has global scope are part of the well-controlled, modular interface. The naming and location where the function is prototyped is a critical part of the modular architecutre. arch.h defines the common, generic interface between ALL archtectures and the rest of the OS.

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

No branches or pull requests

2 participants