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.

Sign up for GitHub

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

Jump to bottom

driver/enc28j60: cleanup #9887

Merged
merged 2 commits into from
Jan 9, 2019
Merged

driver/enc28j60: cleanup #9887

merged 2 commits into from
Jan 9, 2019

Conversation

smlng
Copy link
Member

@smlng smlng commented Sep 5, 2018

Contribution description

Some cleanup and optimisation of the ethernet driver. Safes overall 12 bytes, too.

Testing procedure

Issues/PRs references

@smlng smlng added Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 5, 2018
@smlng smlng force-pushed the pr/driver/enc28j60 branch from 3a5d891 to fc5443a Compare September 5, 2018 09:55
kaspar030
kaspar030 requested changes Sep 5, 2018
View reviewed changes
kaspar030
kaspar030 previously requested changes Sep 5, 2018
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

If you remove the bank change, to me this is good to go.

drivers/enc28j60/enc28j60.c
if ((bank < 0) || (dev->bank == bank)) {
assert(bank < 0x04);

if (bank < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this semantic change split out (into another PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it separate commit

smlng added 2 commits September 5, 2018 13:26
@smlng
drivers/enc28j60: cleanup
4759f69 
Simplify usage of params via MACROs and copy params struct instead
of (re)assigning values to driver struct. Overall code cleanup.
@smlng
drivers/enc28j60: always set memory bank, if valid
f35e478 
Simplify handling of memory banks, ie. remove check if current bank
is target bank and set it explicitly every time.
@smlng smlng force-pushed the pr/driver/enc28j60 branch from fc5443a to f35e478 Compare September 5, 2018 11:26
@smlng
Copy link
Member Author

smlng commented Oct 5, 2018

ping @kaspar030, I put changes into distinct/independent commits, IMHO separate PRs is a bit overkill here.

@PeterKietzmann
Copy link
Member

To me the code change looks ok. I ran a simple test with an enc28j60 shield connected to a samr21-xpro using this change:

diff --git a/examples/gnrc_networking/Makefile b/examples/gnrc_networking/Makefile
index 86b7a94c14..a3250f4b42 100644
--- a/examples/gnrc_networking/Makefile
+++ b/examples/gnrc_networking/Makefile
@@ -40,6 +40,23 @@ USEMODULE += netstats_rpl
 # development process:
 DEVELHELP ?= 1
 
+ifneq (,$(filter samr21-xpro,$(BOARD)))
+    USEMODULE += enc28j60
+    GNRC_NETIF_NUMOF := 2
+
+# fallback: set SPI bus and pins to default values
+    ENC_SPI ?= SPI_DEV\(1\)
+    ENC_CS  ?= GPIO_PIN\(0,18\)
+    ENC_INT ?= GPIO_PIN\(0,28\)
+    ENC_RST ?= GPIO_PIN\(0,13\)
+    # export SPI and pins
+    CFLAGS += -DENC28J60_PARAM_SPI=$(ENC_SPI)
+    CFLAGS += -DENC28J60_PARAM_CS=$(ENC_CS)
+    CFLAGS += -DENC28J60_PARAM_INT=$(ENC_INT)
+    CFLAGS += -DENC28J60_PARAM_RESET=$(ENC_RST)
+endif
+
+
 # Uncomment the following 2 lines to specify static link lokal IPv6 address
 # this might be useful for testing, in cases where you cannot or do not want to
 # run a shell with ifconfig to get the real link lokal address.

Changing the MAC address of the device via ifconfig 8 set addr 11:22:33:44:55:66 worked.

@kaspar030 are you still around? I think your comments were addressed. Please remove your change request.

@PeterKietzmann PeterKietzmann added Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Jan 7, 2019
@PeterKietzmann
Copy link
Member

I'd prefer this semantic change split out (into another PR).

@kaspar030 the bank change was split out into a separate commit which I think is totally fine. Not to bother you too much with this PR and to get it in the upcoming release I consider dismissing your change request tomorrow.

@PeterKietzmann PeterKietzmann added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Jan 9, 2019
PeterKietzmann
PeterKietzmann approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

I re-reviewed the PR and did basic testing that involved SPI transfers and give my ACK. @kaspar030 I'll dismiss your review now. Please give me a sign if you think something hasn't been addressed properly. I'll take care of the follow-up then.

@PeterKietzmann PeterKietzmann dismissed kaspar030’s stale review January 9, 2019 09:31

Comments were addressed and author is not responsive anymore.

@PeterKietzmann PeterKietzmann merged commit 3cf8b4d into RIOT-OS:master Jan 9, 2019
@aabadie aabadie added this to the Release 2019.01 milestone Jan 11, 2019
@smlng smlng deleted the pr/driver/enc28j60 branch June 25, 2019 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeterKietzmann PeterKietzmann PeterKietzmann approved these changes

@kaspar030 kaspar030 kaspar030 left review comments

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2019.01
Development

Successfully merging this pull request may close these issues.
4 participants
@smlng @PeterKietzmann @kaspar030 @aabadie