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

Netump Initial commit #7527

Merged
merged 6 commits into from
Aug 30, 2020
Merged

Netump Initial commit #7527

merged 6 commits into from
Aug 30, 2020

Conversation

hreintke
Copy link
Contributor

This is a continuation from #6518

To ease tracking I first commit the same code, updates come with next commits.

@hreintke
Copy link
Contributor Author

Reply to comments @d-a-v @devyte

On default constructor/deconstructor being generated by compiler.
Need to include when there are also other (de)constructors, even when empty.

PacketType destructor
No need for this, if the destructor is the default (empty body in the cpp)

Done

PacketType constructor

Needed -> Not done

ntoh16
I meant: rather than reimplement here, why not call the lwip functions directly? or wrap the lwip calls in these methods.

These are netdump specific. wrapping lwip calls only complicates the code.
Not done

NetdumpIP
What I mean is that there doesn't seem to be an equivalent for IPv6, and also: is this constructor valid for the case of a sketch with only IPv6? If the answer to that is "no", then should the constructor have different args? have an overload with different args? something else?

NetdumpIP is created because lwip callback ans as such netdump always will have both IPv4 ans IPv6. Regardless of the lwip seting.
No change

Netdump IP (de)constructor.

Constructor needed -> not done
Deconstructor -> done

tcpBufferSize* (uppercase S)

Done

Does it also not apply to WiFiClientSecure?

Do not understand your comment.

Given that what comes next is an infinite loop, I suggest: "WiFiFailed, stopping sketch". I've been confused by these things myself.

Done

The switch was changed, but the arg is still received as an int.

Done

d-a-v
d-a-v approved these changes Aug 21, 2020
View reviewed changes
Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

Works for me, nice tool for debugging network issues or protocols,
Thanks !

earlephilhower
earlephilhower approved these changes Aug 22, 2020
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

This is very awesome!

Nothing that would stop me merging, but a few points you may want to look at for your own edification.

libraries/Netdump/src/NetdumpIP.cpp

bool NetdumpIP::fromString4(const char *address)
{
// TODO: (IPv4) add support for "a", "a.b", "a.b.c" formats
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not int a,b,c,d; int elems = scanf("%d.%d.%d.%d", &a, &b, &c, &d); ...?

libraries/Netdump/library.properties
@@ -0,0 1,9 @@
name=NetDump
version=2
author=Herman Reintke
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your call, but normally there's an email or something else to direct people (@hreintke even) in these fields.

libraries/Netdump/keywords.txt
@@ -0,0 1 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to add the new KEYWORD1/2s. Not a big deal.

libraries/Netdump/src/NetdumpIP.cpp
uint32_t acc = 0; // Accumulator
int dots = 0, doubledots = -1;

while (*address)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as IPV4, but maybe it's not so easy with potential empty slots (i.e. fe:aa2::::...)

libraries/Netdump/src/NetdumpIP.cpp
}
return n;
}
for (int i = 0; i < 4; i )
Copy link
Collaborator

Choose a reason for hiding this comment

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

p.printf("%d.%d.%d.%d")?

libraries/Netdump/src/NetdumpIP.h
/*
NetdumpIP.h

Created on: 18 mei 2019
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you like to include the GPL and your full name here?

libraries/Netdump/src/NetdumpPacket.cpp
{
for (int i = 0; i < 6; i )
{
sstr.printf_P(PSTR("x"), (unsigned char)data[dataIdx i]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're using printf_P(PSTR("x")), can we use it elsewhere where printf(x") is? Now we'll end up with copies in RODATA, TEXT, and IROM.PSTR...

@earlephilhower
Copy link
Collaborator

This is too valuable to wait for minor tweaks. Merging and will open an issue pointing to some of the comments herein.

@earlephilhower earlephilhower merged commit be812d2 into esp8266:master Aug 30, 2020
@earlephilhower earlephilhower mentioned this pull request Aug 30, 2020
Open
davisonja added a commit to davisonja/Arduino that referenced this pull request Sep 10, 2020
@davisonja
Merge branch 'master' into ota_commands_in_flash
3ebeb26 
* master: (299 commits)
  Fix error message typo (esp8266#7581)
  Update certs-from-mozilla.py (esp8266#7578)
  Update DigestAuthorization.ino (Simple example update) (esp8266#7579)
  Fix gzip signed OTA error (esp8266#7577)
  Properly replace toolchain in PlatformIO CI script (esp8266#7580)
  Update certs-from-mozilla.py (esp8266#7573)
  Fixup weird combination of oneline/multi line comments (esp8266#7566)
  Reduce codesize of setOutputPower (esp8266#7572)
  Fix typos in tests
  Force gcc inlining, use same style for getCycleCount as for getCpuFreqMHz.
  Even more concise #if form.
  Inline, fewer LOC, remove redundant definition in cpp.
  Netump Initial commit (esp8266#7527)
  Delete owner field (esp8266#7563)
  Avoid float-double-conversion (esp8266#7559)
  Use direct member initialization instead of ctr initialisation (esp8266#7556)
  Add CI test for eboot build (esp8266#7546)
  getCpuFreqMHz(): fix when F_CPU is not defined (esp8266#7554)
  emulation-on-host makefile update, allowing to pass more options (esp8266#7552)
  add sdk options to "generic esp8285 module" (esp8266#7550)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-a-v d-a-v d-a-v approved these changes

@earlephilhower earlephilhower earlephilhower approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hreintke @earlephilhower @d-a-v