[FW-79]- Creation of ethernet Mock #438

g0nz4I0 · 2024-10-30T22:32:04Z

No description provided.

g0nz4I0

Overall looks good, I dont understand the purpose of the timeout, and if you can install callbacks maybe we can do that? instead of the thread. IPV4 interface I like the small changes

Remove all the #ifdef guards
I like the std::lock_guard

Inc/HALALMock/Models/IPV4/IPV4.hpp

Inc/HALALMock/Services/Communication/Ethernet/Ethernet.hpp

Inc/HALALMock/Services/Communication/Ethernet/TCP/ServerSocket.hpp

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

Src/HALALMock/Services/Communication/Ethernet/Ethernet.cpp

jmaralo

There are some corner cases that I pointed out that I got after working two years with sockets from Go which might get you if you don't know them.

I also thing there is a lot of refactoring that can be done to make the code much cleaner and easy to follow. Is easy for the code to get messy with all the syscalls and error handling ifs.

jmaralo · 2024-10-31T17:41:40Z

Inc/HALALMock/Services/Communication/Ethernet/TCP/Socket.hpp

+	queue<vector<uint8_t>> tx_packet_buffer;
+	queue<vector<uint8_t>> rx_packet_buffer;


I do think it will be better (and less obscure what you are sending) by storing a pointer to a packet

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

jmaralo · 2024-10-31T18:17:08Z

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

+	fd_set write_fds;
+	FD_ZERO(&write_fds);
+	FD_SET(socket_fd, &write_fds);
+	//timeout
+	struct timeval timeout;
+	timeout.tv_sec = 5; 
+	timeout.tv_usec = 0;
+	//use select to check if fthe socket has established connection
+	int result = select(socket_fd + 1, NULL, &write_fds, NULL, &timeout);
+	if (result > 0 && FD_ISSET(socket_fd, &write_fds)) {
+		//If it's connected we do as connected_callback
+		if(connecting_sockets.contains(remote_node)){
+			connecting_sockets.erase(remote_node);
+			state = CONNECTED;
+		}
+	}


Don't know if this will even work, what happens if the client connects after 5s? will the connection_callback ever be called?

There is a cleaner way, you can use poll (more modern than select) inside a thread. Basically you will create a thread like this:

// check beforehand the connection worked std::thread wait_for_connection([&](){ // create here the pollfs struct in a variable called socket_event int result = poll(socket_event, 1, -1); // -1 means to never timeout // check the result here connection_callback(); }); // don't join the thread, store it somewhere or even deatatch it

fill in the blanks and it should work well

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

Src/HALALMock/Services/Communication/Ethernet/Ethernet.cpp

Src/HALALMock/Services/Communication/Ethernet/EthernetNode.cpp

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

Inc/HALALMock/Services/Communication/Ethernet/TCP/Socket.hpp

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

g0nz4I0 · 2024-11-03T09:40:49Z

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

 	while (!tx_packet_buffer.empty()) {
-        vector<uint8_t> packet = tx_packet_buffer.front();
-        ssize_t sent_bytes = ::send(socket_fd, packet.data(), packet.size(), 0);
+        HeapPacket packet = tx_packet_buffer.front();


Pointer or reference type, Packet* will do

g0nz4I0 · 2024-11-03T09:43:08Z

⚠️ Cuando tienes tipos polimórficos tienes que utilizarlos como pointer types, esto es, como una referencia o como un puntero, si no no te va a funcionar, Packet en este caso es la clase base de todo, es decir, todos heredan de packet.⚠️

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

g0nz4I0 · 2024-11-03T12:04:43Z

Src/HALALMock/Services/Communication/Ethernet/TCP/Socket.cpp

 	while (!tx_packet_buffer.empty()) {
-        HeapPacket packet = tx_packet_buffer.front();
-        ssize_t sent_bytes = ::send(socket_fd, packet.build(), packet.get_size(), 0);
+        HeapPacket *packet = tx_packet_buffer.front();


can you explain this, at the end packet is a virtual class to store data shouldn't I use one class derivated from packet

jmaralo

Gave it a quick read and didn't find anything weird, keep going at it so we can start seeing where compilation fails. But I'll point out just two things

jmaralo · 2024-11-05T22:09:08Z

Src/HALALMock/Services/Communication/Ethernet/TCP/ServerSocket.cpp

+	//to reuse local address:
+	int opt = 1;
+	if (setsockopt(server_socket_fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) {
+    	std::cerr << "Error setting SO_REUSEADDR\n";
+   		close(server_socket_fd);
+    	return;
+	}
+	//disable naggle algorithm
+	int flag = 1;
+	if (setsockopt(server_socket_fd,IPPROTO_TCP,TCP_NODELAY,(char *) &flag, sizeof(int)) < 0){
+		std::cout<<"It has been an error disabling Nagle's algorithm\n";
+		close(server_socket_fd);
+		return;
+	}
+	//habilitate keepalives
+    int optval = 1;
+    if (setsockopt(server_socket_fd, SOL_SOCKET, SO_KEEPALIVE, &optval, sizeof(optval)) < 0) {
+        std::cout << "ERROR configuring KEEPALIVES\n";
+        close(server_socket_fd);
+        return;
+    }
+	// Configurar TCP_KEEPIDLE it sets what time to wait to start sending keepalives 
+    float tcp_keepidle_time = static_cast<float>(keepalive_config.inactivity_time_until_keepalive_ms)/1000.0;
+    if (setsockopt(server_socket_fd, IPPROTO_TCP, TCP_KEEPIDLE, &tcp_keepidle_time, sizeof(tcp_keepidle_time)) < 0) {
+        std::cout << "Error configuring TCP_KEEPIDLE\n";
+		close(server_socket_fd);
+        return;
+    }
+	  //interval between keepalives
+    float keep_interval_time = static_cast<float>(keepalive_config.space_between_tries_ms)/1000.0;
+    if (setsockopt(server_socket_fd, IPPROTO_TCP, TCP_KEEPINTVL, &keep_interval_time, sizeof(keep_interval_time)) < 0) {
+        std::cout << "Error configuring TCP_KEEPINTVL\n";
+        close(server_socket_fd);
+        return;
+    }
+	 // Configure TCP_KEEPCNT (number keepalives are send before considering the connection down)
+	 float keep_cnt = static_cast<float>(keepalive_config.tries_until_disconnection)/1000.0;
+    if (setsockopt(server_socket_fd, IPPROTO_TCP, TCP_KEEPCNT, &keep_cnt, sizeof(keep_cnt)) < 0) {
+        std::cout << "Error to configure TCP_KEEPCNT\n";
+        close(server_socket_fd);
+        return ;
+    }


I would still separate into more functions, but just a nitpick, also, return a bool that is true if the configuration was successful or false in any other case

Src/HALALMock/Services/Communication/Ethernet/UDP/DatagramSocket.cpp

…changes in serverSocket to only have one connection

oganigl · 2024-11-11T22:23:13Z

I don't know how to wite a description of the pr once I changed it from draft to open, so I will leave this message explaining the pr. During this pr I've tried to convert the code from the lwip to sockets in linux. But always trying to replicate the behave of the halal. we will have to create test to assure the well behave of the pr. The changes have been done in these files: Ethernet, Pin,IPV4, Socket, ServerSocket and DatagramSocket

jmaralo

I'm approving the PR because it has been open for too long. There are a few things that won't work with ServerSocket from the way it worked with LWIP and the way it works in linux. I think we should merge this PR and fix those things later on with new jira tickets

jmaralo · 2024-11-15T16:58:13Z

Inc/HALALMock/Services/Communication/Ethernet/UDP/DatagramSocket.hpp

+		if(sendto(udp_socket,packet_buffer,size,0,(struct sockaddr *)&remote_socket_addr, sizeof(remote_socket_addr)) < 0){
+			close(udp_socket);
+			return false;
+		}


sendto might not send all the data in one go, you should do a while loop until all the data is sent

jmaralo · 2024-11-15T17:00:13Z

Src/HALALMock/Services/Communication/Ethernet/TCP/ServerSocket.cpp

 ServerSocket::ServerSocket(IPV4 local_ip, uint32_t local_port) : local_ip(local_ip),local_port(local_port){
 	if(not Ethernet::is_running) {
-		ErrorHandler("Cannot declare UDP socket before Ethernet::start()");
+		cout<<"Cannot declare UDP socket before Ethernet::start()\n";


ErrorHandler

jmaralo · 2024-11-15T17:02:38Z

Src/HALALMock/Services/Communication/Ethernet/TCP/ServerSocket.cpp

+	// Clean all descriptors
+	 if (client_fd != -1) {
+        ::close(client_fd);
+        client_fd = -1;
+    }
+    if (server_socket_fd != -1) {
+        ::close(server_socket_fd);
+        server_socket_fd = -1;
+    }
+	//clean the transmisions buffers
+	while (!tx_packet_buffer.empty()) {
+        tx_packet_buffer.pop();
+    }
+    while (!rx_packet_buffer.empty()) {
+        rx_packet_buffer.pop();
+    }
+	//eliminate the threads
+	if(state == LISTENING){
+		~listening_thread();
+	}else if(state == ACCEPTED){
+		~receive_thread();


I dont know if closing both in the close method and in the destructor will give any problems, because usually doing a double close returns an error

also see that in the HALAL the implementation is different for close and for the destructor

…e in the first attempt

oganigl

I think this pr it has to been check by testing it.

g0nz4I0 commented Oct 30, 2024

View reviewed changes

jmaralo requested changes Oct 31, 2024

View reviewed changes

jmaralo assigned oganigl Oct 31, 2024

g0nz4I0 commented Nov 3, 2024

View reviewed changes

jmaralo reviewed Nov 5, 2024

View reviewed changes

oganigl added 6 commits November 7, 2024 22:24

add bool configure_socket

a1498b9

change message to english

5789dd4

eliminate socket_fd move

9444faf

continue with serverSocket, talked with stefan and I have to do some …

972dfa8

…changes in serverSocket to only have one connection

Finish implementation ServerSocketMock. Now waiting for feedback

586b345

eliinate endif

1205330

oganigl marked this pull request as ready for review November 11, 2024 22:15

oganigl changed the title ~~Feature/fw 79~~ Feature/fw 79- Creation of ethernet Mock Nov 11, 2024

oganigl changed the title ~~Feature/fw 79- Creation of ethernet Mock~~ [FW-79]- Creation of ethernet Mock Nov 12, 2024

jmaralo approved these changes Nov 15, 2024

View reviewed changes

eliminate some #endif that I didn't see before

d646c28

oganigl force-pushed the feature/fw-79 branch from 325618a to d646c28 Compare November 16, 2024 09:13

oganigl added 5 commits November 16, 2024 10:16

change datagramsocket to resend if it didn't send the complete messag…

66e5041

…e in the first attempt

change socket.hpp keepalives

7095930

change keepalive linux in serverSockets

a67ba3a

forgot to erase some ms variables

6834eec

eliminate mpu_start from ethernet

281b193

oganigl approved these changes Nov 16, 2024

View reviewed changes

Merge branch 'development' into feature/fw-79

52e8e20

g0nz4I0 merged commit 3ead685 into development Nov 16, 2024

g0nz4I0 deleted the feature/fw-79 branch November 16, 2024 10:17

		queue<vector<uint8_t>> tx_packet_buffer;
		queue<vector<uint8_t>> rx_packet_buffer;

[FW-79]- Creation of ethernet Mock #438

[FW-79]- Creation of ethernet Mock #438

Uh oh!

Conversation

g0nz4I0 commented Oct 30, 2024

Uh oh!

g0nz4I0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmaralo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g0nz4I0 commented Nov 3, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmaralo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oganigl commented Nov 11, 2024

Uh oh!

jmaralo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oganigl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants