Feature hal gpio

[riansanderson]

I moved these changes off into their own branch and issued this pull request so we can properly code review them

[riansanderson]

@ravibadam and @pankajgoenka please review this pull request

[hyau]

Commit comments:
For commit - 4d856bf:

1. Please seperate optimization/cleanups from functional changes. The used variable removal should be seperate.
2. What is the reason for adding const to the ASF_TASK_ARG define? It does not seem to fit any of the descriptions.
3. What is the reason for the change of datatype for the priorty member? Changing to signed seems to be a bug fix? priorty does not seem to be related to HALs. Please clarify as to the relevance to this commit.
4. Since this is source controlled, please remove instead of commented out old lines. Diffs will show what the old code is just fine. Keeping it commented out pollutes the code.
5. In Gyro_HardwareSetup() (and other drivers):
   (a) why the additional block/scoping? It is a small function already. Justify or remove please.
   (b) Why are the other CHIP* calls still present? They are related to GPIOs and if the HAL doesn't cover them, prehaps the HAL should be revisited. An embedded use HAL targeting power sensitive apps should support power management primatives.
   (c) Why are pinmux options not in the HAL?
6. Why the stack size change? It is not described in the commit.
7. hostif.c - Does it make sense from a size stand point to keep the gpio_t variable in RAM? Prehaps move it to a static constant?
8. Instead of all the attibute unused scattered all over, can we please put them into a define. i.e. #define UNUSED __attribute....
   This way if we move to another compiler, we don't have to touch all the files.
9. Please drop all these new typedefs. It makes code hard to follow. If it is a structure, call it a structure. Despite the typedefs, the code freely accesses members which bypasses any so call
   abstraction that might be created.

commit aa5c1a2:

1. Why the wholesale change in common.h, debugprintf.c, hw_setup.h, etc? They make the change complete unreviewable. If there is a need for a whitespace change, please:
   (a) Make a commit with solely white space changes and label as such. A white space ignoring
   diff should show NO changes whatsoever.
   (b) Seperate out any functional changes into another commit.
2. There seems to be a lot of changes beyond what is described. Samples include:
   (a) Change in the mutexes
   (b) How is adding a ready flag in the acq task relate to this commit?
   As provided (1) is sufficient and strong enough reason to completely reject this change as anything can be snucked in and would be impossible to sort out without a lot of time.

Style comments:

1. Please do not indent the description part. Git will handle the the identation by itself.
2. A signed-off by line would be greatly appreciated. This is so credit/fault canbe assigned as
   cherry-picks, merges, etc may change the user git records.
3. ALWAYS setup your git user with a globally valid identifier. Using a .local machine name is a bad
   idea.
4. Please keep the white space system consistant in a file. If a file has hard tabs, please use that. Recommend Linux kernel coding style just because it is well documented and tools are available to scrub commits.

[pankajgoenka]

Hi Hunyue,

Thank you for the review comments.
Please find my reply as follows:

1. Please separate optimization/cleanups from functional changes. The used variable removal should be seperate.

Sure. I will separate them out.

1. What is the reason for adding const to the ASF_TASK_ARG define? It does not seem to fit any of the descriptions.

The arguments to a task/thread are generally expected to remain unchanged so I thought making it const makes more sense.
I will add this change as part of CMSIS commit and add a note in commit log.

1. What is the reason for the change of datatype for the priorty member? Changing to signed seems to be a bug fix? priorty does not seem to be related to HALs. Please clarify as to the relevance to this commit.

Priority of threads should be signed as per CMSIS.
This change should have been made along with the CMSIS changes.

1. Since this is source controlled, please remove instead of commented out old lines. Diffs will show what the old code is just fine. Keeping it commented out pollutes the code.

Sure. Will do that.

1. In Gyro_HardwareSetup() (and other drivers):
   (a) why the additional block/scoping? It is a small function already. Justify or remove please.

Agreed. Will remove the additional scope.

(b) Why are the other CHIP* calls still present? They are related to GPIOs and if the HAL doesn't cover them, prehaps the HAL should be revisited. An embedded use HAL targeting power sensitive apps should support power management primatives.

Chip_SYSCON calls were not replaced as part of GPIO HAL implementation since HAL APIs for the same are not defined in mbed.
Other CHIP* APIs shall be removed as and when HAL APIs of other modules are committed.

(c) Why are pinmux options not in the HAL?

HAL APIs for pinmux are not defined in mbed. pinmap is used to achieve the same.

1. Why the stack size change? It is not described in the commit.

Stack overflowed after introducing the HAL code. The stack sizes were therefore readjusted.
I'll add a note in commit log.

1. hostif.c - Does it make sense from a size stand point to keep the gpio_t variable in RAM? Prehaps move it to a static constant?

I'll make it global const instead of static const since it is used across multiple files

1. Instead of all the attibute unused scattered all over, can we please put them into a define. i.e. #define UNUSED __attribute....
   This way if we move to another compiler, we don't have to touch all the files.

Sure. Will do that.

1. Please drop all these new typedefs. It makes code hard to follow. If it is a structure, call it a structure. Despite the typedefs, the code freely accesses members which bypasses any so call
   abstraction that might be created.

The existing coding convention in OSP and mbed is followed for the above.

commit aa5c1a2:

1. Why the wholesale change in common.h, debugprintf.c, hw_setup.h, etc? They make the change complete unreviewable. If there is a need for a whitespace change, please:
   (a) Make a commit with solely white space changes and label as such. A white space ignoring
   diff should show NO changes whatsoever.
   (b) Seperate out any functional changes into another commit.

It seems the file format is changed from UNIX to DOS, which is why the diff shows the complete file as changes.
I didn't realize that Keil uVision automatically changes EOL to windows format at the time of saving a file.
I'll correct that.

1. There seems to be a lot of changes beyond what is described. Samples include:
   (a) Change in the mutexes

These changes are part of the addition of RTOS abstraction layer (CMSIS). We haven't changed mutexes. Just replaced RTX implementation with CMSIS implementation.

(b) How is adding a ready flag in the acq task relate to this commit?

With the addition of CMSIS layer, the initialization time of different tasks changed.
We realized that it was necessary to wait for the acq task to finish initialization before sending messages.
I'll mention the same in commit message.

As provided (1) is sufficient and strong enough reason to completely reject this change as anything can be snucked in and would be impossible to sort out without a lot of time.

I'll ammend/rebase the commits to incorporate the above mentioned changes.

Style comments:

1. Please do not indent the description part. Git will handle the the identation by itself.
2. A signed-off by line would be greatly appreciated. This is so credit/fault can be assigned as
   cherry-picks, merges, etc may change the user git records.
3. ALWAYS setup your git user with a globally valid identifier. Using a .local machine name is a bad idea.
4. Please keep the white space system consistent in a file. If a file has hard tabs, please use that. Recommend Linux kernel coding style just because it is well documented and tools are available to scrub commits.

Sure. I'll take of those.

-Pankaj

[hyau]

Thanks. The main thing with most of the comments is there were changes not documented in the commit log. From a reviewer point of view, small well documented commits are easier to review and
verify then large commits. It also makes it easier for debugging as breaking it lets you leverage things like git bisect. The only requirement is each commit must compile and serves one purpose (be it white space or API change)

Can you point to the coding convention for OSP or mbed that requires you to typedef structures and then directly access members within them? Not trying to create a new debate but that makes little sense from a reviewing point of view. All it does is add extra level of indirection.

[audienceit]

How do I get off of this email distribution?

Jamie Raible

From: hyau [mailto:notifications@github.com]
Sent: Friday, July 31, 2015 3:11 PM
To: sensorplatforms/osp
Subject: Re: [osp] Feature hal gpio (#1)

Thanks. The main thing with most of the comments is there were changes not documented in the commit log. From a reviewer point of view, small well documented commits are easier to review and
verify then large commits. It also makes it easier for debugging as breaking it lets you leverage things like git bisect. The only requirement is each commit must compile and serves one purpose (be it white space or API change)

Can you point to the coding convention for OSP or mbed that requires you to typedef structures and then directly access members within them? Not trying to create a new debate but that makes little sense from a reviewing point of view. All it does is add extra level of indirection.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-126824911.

[pankajgoenka]

Can you point to the coding convention for OSP or mbed that requires you to typedef structures and then directly access members within them? Not trying to create a new debate but that makes little sense from a reviewing point of view. All it does is add extra level of indirection.

I understand your point. There is no document for mbed or OSP coding convention that I know of. However, most of these typedef-ed structures are pre-defined in mbed header files. The implementation of HAL APIs would be very difficult without accessing the members of these structs. Maybe we can discuss and take a call about whether this needs to be changed.

I've closed the review comments and committed the modified code as separate branches as discussed with Rian. I've raised pull requests again for the same. Request you to kindly review.

CMSIS Changes: Pull Request #2
GPIO and PINMAP HAL Changes: Pull Request #3

In addition I have also committed the RTC HAL APIs as Pull Request #4

-Pankaj