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

scan_hosts rework/expansion #353

Merged
merged 1 commit into from
Oct 24, 2024
Merged

scan_hosts rework/expansion #353

merged 1 commit into from
Oct 24, 2024

Conversation

Nolven
Copy link
Contributor

@Nolven Nolven commented Oct 23, 2024

Proposed Changes

Instead of just scanning ports, the same function takes the role of wifi client scanner, parsing additional info.
Tbh it can be expanded to mDNS and DHCP parsing, but I wanna keep mr short

Types of Changes

  • optimized by removing redundant requests
  • fixed number of hosts calculation
  • added mac identification
  • added oui query by mac to identify the manufacturer
  • option scanPorts renamed to host info

Verification

Connect to wifi -> scan hosts -> check that (GATE) label is present
Select one of the hosts, check that labels look alright

Testing

I might add a simulator and CI but after adding encapsulation and when other wanna-have list is exausted

Linked Issues

None

User-Facing Change

Pic diff
photo_2024-10-23_19-26-26
host_screen


Further Comments

@@ -5,140 +5,100 @@
#include "core/wifi_common.h"
#include "scan_hosts.h"
#include "clients.h"
#include <ESPping.h>
#include <HTTPClient.h>
#include <WiFi.h>
Copy link
Owner

@pr3y pr3y Oct 23, 2024

Choose a reason for hiding this comment

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

Thanks for contributing! i liked this Manufacturer feature and i think its OK to use api.maclookup.app to get these Host info, here are some suggestions to be able to merge it:

ESPping.h and WiFi.h are already declared in scan_hosts.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated according to the principle include what you use

// TODO: move to a config
// gives hosts time to respond
// from the tests with the value under 4k high-end hosts in subnet \24 are not able to respond
TickType_t beforeReadingArpDelay = 5000u / portTICK_PERIOD_MS;
Copy link
Owner

Choose a reason for hiding this comment

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

I tried to make less delay, maybe this can be smaller.
The current release build can probe the 254 hosts in approximately 4 seconds, after your PR even before removing the double send_arp, it looks like it takes approximately 10 seconds to probe all hosts...
Im not sure if thats the cause of it.

Copy link
Contributor Author

@Nolven Nolven Oct 24, 2024

Choose a reason for hiding this comment

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

Yeah, it was combination of 4s delay between sending and reading + 40ms between each request
I've completely reworked reading function to consider cached requests limitation, so now it's much quicker

If it's not a problem, could you check on your network that all required hosts are found? My initial reasoning behind adding that timeout is that not all hosts were found

* optimizied by removing redundant requests
* fixed number of hosts calculation
* added mac identification
* added oui query by mac to identify manufacturer
* option scanPorts renamed to host info
@pr3y pr3y merged commit 76c978c into pr3y:main Oct 24, 2024
3 checks passed
@A9AG
Copy link

A9AG commented Nov 11, 2024

i flashed bruce on my t embed beta and its buggey its cool tho and how come the RGB cog light dosent come one can you fix that

@Nolven
Copy link
Contributor Author

Nolven commented Nov 11, 2024

@A9AG can you send a photo and explain what exactly is bugging? I only have a cardputer to test on

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

Successfully merging this pull request may close these issues.

3 participants