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

Multiport linux support #303

Merged
merged 1 commit into from
Feb 2, 2021
Merged

Conversation

olbjo
Copy link
Collaborator

@olbjo olbjo commented Jan 15, 2021

  • See doc/multiple_ports.rst for information on how to run configure
    and run a mulitport configuration
  • Verified on Raspberry Pi with additional USB ethernet interfaces

@olbjo olbjo requested a review from pyhys January 15, 2021 15:00
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!
I think we need to enable filtering of incoming packets dependent on Ethertype already in the Linux kernel, otherwise it will be lots of work for the application. See os_eth_init() in https://github.com/rtlabs-com/p-net/blob/6670292041fd85c028ed1d40c22d5b1ced2fd882/src/osal/linux/osal_eth.c

doc/multiple_ports.rst Outdated Show resolved Hide resolved
sample_app/sampleapp_common.h Outdated Show resolved Hide resolved
sample_app/sampleapp_common.c Outdated Show resolved Hide resolved
sample_app/sampleapp_common.c Outdated Show resolved Hide resolved
sample_app/sampleapp_common.c Outdated Show resolved Hide resolved
src/common/pf_eth.c Outdated Show resolved Hide resolved
src/common/pf_eth.c Outdated Show resolved Hide resolved
src/common/pf_lldp.h Outdated Show resolved Hide resolved
src/common/pf_eth.c Outdated Show resolved Hide resolved
src/ports/rt-kernel/pnal_eth.c Outdated Show resolved Hide resolved
@olbjo
Copy link
Collaborator Author

olbjo commented Jan 18, 2021

Rebased on latest master

src/pnal.h Outdated Show resolved Hide resolved
@olbjo
Copy link
Collaborator Author

olbjo commented Jan 19, 2021

Rebased

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!
Mostly trivial changes

sample_app/sampleapp_common.c Outdated Show resolved Hide resolved
sample_app/sampleapp_common.h Outdated Show resolved Hide resolved
sample_app/sampleapp_common.h Outdated Show resolved Hide resolved
sample_app/sampleapp_common.h Outdated Show resolved Hide resolved
sample_app/sampleapp_common.h Outdated Show resolved Hide resolved
src/ports/linux/sampleapp_main.c Outdated Show resolved Hide resolved
src/ports/rt-kernel/pnal_sys.h Outdated Show resolved Hide resolved
src/ports/rt-kernel/sampleapp_main.c Outdated Show resolved Hide resolved
src/device/pf_port.c Outdated Show resolved Hide resolved
src/device/pnet_api.c Outdated Show resolved Hide resolved
src/common/pf_eth.h Outdated Show resolved Hide resolved
src/common/pf_eth.h Outdated Show resolved Hide resolved
src/pnal.h Outdated Show resolved Hide resolved
src/common/pf_lldp.c Outdated Show resolved Hide resolved
src/pnal.h Outdated Show resolved Hide resolved
src/common/pf_lldp.c Outdated Show resolved Hide resolved
src/common/pf_lldp.h Outdated Show resolved Hide resolved
src/common/pf_lldp.h Outdated Show resolved Hide resolved
src/device/pf_port.h Outdated Show resolved Hide resolved
src/common/pf_eth.h Outdated Show resolved Hide resolved
@olbjo
Copy link
Collaborator Author

olbjo commented Jan 25, 2021

Review comments fixed.
I have also run regression tests on a raspberry pi with a single port configuration without seeing any problems. (Not all tests in suite run).

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 changes for Ethernet, LLDP and port handling. Didn't look at code for sample app and unit tests.

sample_app/sampleapp_common.c Outdated Show resolved Hide resolved
src/common/pf_eth.c Outdated Show resolved Hide resolved
src/common/pf_eth.c Outdated Show resolved Hide resolved
src/common/pf_eth.h Show resolved Hide resolved
src/common/pf_lldp.h Outdated Show resolved Hide resolved
src/device/pf_port.h Outdated Show resolved Hide resolved
src/device/pf_port.h Outdated Show resolved Hide resolved
src/device/pnet_api.c Outdated Show resolved Hide resolved
src/pnal.h Outdated Show resolved Hide resolved
src/ports/rt-kernel/pnal_eth.c Outdated Show resolved Hide resolved
@olbjo
Copy link
Collaborator Author

olbjo commented Jan 26, 2021

Review comments fixed

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.

Well done!

@pyhys pyhys requested a review from hefloryd January 26, 2021 13:52
@olbjo olbjo mentioned this pull request Jan 27, 2021
for test. */
char c;

memset (p_if_list, 0, sizeof (*p_if_list));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be simpler to use strtok(_r)? here instead? Something along the lines of:

   char * argv[16];
   char * p;
   char * saveptr;

   /* Split line into argv using space as delimiter */
   p = strtok_r (line, " ", &saveptr);
   while (p != NULL && argc < NELEMENTS (argv) - 1)
   {
      argv[argc++] = p;
      p = strtok_r (NULL, " ", &saveptr);
   }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, possibly. My first idea was to use strtok, but then I needed to copy arg_str for strtok to be able to insert '\0' and I also need to copy the strings so I ended up with this implementation.

Do you prefer that I use a strtok-based implementation instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

When possible we should use the standard libraries. Unfortunately strok is not reentrant and strok_r (which is reentrant) might not be portable (it's not available on windows for instance). So let's stick with this for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This network initialization / configuration from the application is messy. We will try to improvement according to #316. In this case this code may disappear.

sample_app/sampleapp_common.h Show resolved Hide resolved
sample_app/sampleapp_common.h Outdated Show resolved Hide resolved
test/mocks.cpp Outdated
@@ -57,8 +58,9 @@ pnal_eth_handle_t * mock_pnal_eth_init (
{
pnal_eth_handle_t * handle;

handle = (pnal_eth_handle_t *)calloc (1, sizeof (pnal_eth_handle_t));
handle = (pnal_eth_handle_t *)calloc (1, 10 /*sizeof (pnal_eth_handle_t)*/);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree this looks strange. See updated mock implementation.

test/mocks.cpp Outdated
@@ -103,6 +105,11 @@ int mock_pnal_eth_send (pnal_eth_handle_t * handle, pnal_buf_t * p_buf)
return p_buf->len;
}

int mock_pnal_eth_recv (void * arg, pnal_buf_t * buf)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems backwards? The purpose of a mock-function is to decouple from external dependencies. This instead adds an external dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have updated the mock implementation.
Done

@@ -101,10 +101,10 @@ TEST_F (DcpTest, DcpHelloTest)

p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE);
memcpy (p_buf->payload, get_name_req, sizeof (get_name_req));
ret = pf_eth_recv (net, p_buf);
ret = mock_pnal_eth_recv (net, p_buf);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is being tested here? Looks like it is a test of the mock-function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have updated the the mock implementation to avoid this.
Done

@olbjo olbjo force-pushed the multi_port_linux branch 2 times, most recently from 3a8e838 to f1de47b Compare February 1, 2021 10:17
@olbjo
Copy link
Collaborator Author

olbjo commented Feb 1, 2021

Rebased branch on master

@@ -59,6 +66,10 @@ pnal_eth_handle_t * mock_pnal_eth_init (

handle = (pnal_eth_handle_t *)calloc (1, sizeof (pnal_eth_handle_t));

handle->if_name = if_name;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of allocating a pnal_eth_handle_t object in this function, perhaps one could initialise mock_os_data.eth_if_handle in mock_init() to some constant pointer, e.g (void*)42, and then return it here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. BTW, doesn't the calloc call leak memory for every test-case using mock_clear()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I assume so. I will update according to Fredriks idea.

@olbjo olbjo force-pushed the multi_port_linux branch 2 times, most recently from 1f5358b to 6ce2de1 Compare February 1, 2021 12:04
/* Initialize everything (and the DCP protocol) */
/* First initialize the network interface */
net->eth_handle =
pnal_eth_init (p_cfg->if_cfg.main_port.if_name, pf_eth_recv, (void *)net);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove #define pnal_eth_init mock_pnal_eth_init also

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, second push below is a rebase on master

- See doc/multiple_ports.rst for information on how to configure
  and run a mulitport configuration
- Verified on Raspberry Pi with additional USB ethernet interfaces
@hefloryd hefloryd merged commit fced618 into rtlabs-com:master Feb 2, 2021
@olbjo olbjo deleted the multi_port_linux branch May 19, 2021 06:33
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