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

Simplify configuration #333

Merged
merged 2 commits into from
Feb 19, 2021
Merged

Conversation

olbjo
Copy link
Collaborator

@olbjo olbjo commented Feb 10, 2021

Implements part of enhancements suggested in #316.

Main change is that ethernet address and port name are not configuration parameters set by sample application.
To review the the update check out changes in pnet_api.h and pf_types.h rest of the updates follows from these updates.

A second commit renames PNET_MAX_PORT to PNET_NUMBER_OF_PHYSICAL_PORTS

Rebased on #332 to avoid future merge conflict.

Copy link
Collaborator

@pyhys pyhys left a comment

Choose a reason for hiding this comment

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

Great work, this really simplifies the usage of the library.
Just a few trivial comments.

src/common/pf_eth.c Outdated Show resolved Hide resolved
src/device/pf_port.c Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@fredrikdanielmoller fredrikdanielmoller left a comment

Choose a reason for hiding this comment

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

For strings in configuration, I think it would be more user-friendly to let them be pointers instead of arrays (i.e. const char* instead of char[N]).

CMakeLists.txt Outdated Show resolved Hide resolved
char port_name[PNET_PORT_NAME_MAX_SIZE]; /**< Terminated string */
uint16_t rtclass_2_status;
uint16_t rtclass_3_status;
pnet_netif_name_t port_netif;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pnet_netif_name_t port_netif;
const char * network_interface_name;

(Assuming that pnet_cfg_t is no longer stored in pnet_t.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove pnet_cfg_t from pnet_t is out of the scope for this task. I will use name "netif_name".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Beware that the copy of pnet_cfg_t in pnet_t will be a shallow copy, unless the strings are also copied.

include/pnet_api.h Outdated Show resolved Hide resolved
include/pnet_api.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@pyhys pyhys left a comment

Choose a reason for hiding this comment

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

Add this to src/device/pf_port.c:

#if PNET_MAX_SUBSLOTS < (2 + PNET_NUMBER_OF_PHYSICAL_PORTS)
#error "DAP requires 2 + PNET_NUMBER_OF_PHYSICAL_PORTS subslots. Increase PNET_MAX_SUBSLOTS."
#endif

@olbjo olbjo force-pushed the simplify_configuration branch 6 times, most recently from b7d6ebf to f8f567e Compare February 16, 2021 07:38
@olbjo
Copy link
Collaborator Author

olbjo commented Feb 16, 2021

Comments handled, rebased on master. @pyhys I think you need to re-review updaes

Copy link
Collaborator

@pyhys pyhys left a comment

Choose a reason for hiding this comment

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

Great!

@olbjo olbjo force-pushed the simplify_configuration branch from f8f567e to 1177f53 Compare February 17, 2021 15:15
Copy link
Collaborator

@fredrikdanielmoller fredrikdanielmoller left a comment

Choose a reason for hiding this comment

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

Reviewed pnet_api.h and pf_types.h

include/pnet_api.h Show resolved Hide resolved
src/pf_types.h Outdated Show resolved Hide resolved
src/pf_types.h Show resolved Hide resolved
src/pf_types.h Outdated Show resolved Hide resolved
src/pf_types.h Outdated Show resolved Hide resolved
This was referenced Feb 18, 2021
Moved initialization of mac addresses and lldp port name to stack.
@olbjo olbjo force-pushed the simplify_configuration branch from 1177f53 to c18e847 Compare February 18, 2021 08:20
@pyhys pyhys requested a review from hefloryd February 18, 2021 10:39
@olbjo olbjo force-pushed the simplify_configuration branch from c18e847 to a58e9dd Compare February 19, 2021 11:43
@olbjo
Copy link
Collaborator Author

olbjo commented Feb 19, 2021

Removed occurrence of PNET_MAX_PORT that emerged due to rebase.

@hefloryd hefloryd merged commit 8784f48 into rtlabs-com:master Feb 19, 2021
@olbjo olbjo deleted the simplify_configuration branch May 19, 2021 06:34
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.

4 participants