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

Add driver for PIC32MX #1203

Closed
wants to merge 1 commit into from

Conversation

vishwamartur
Copy link

Related to #789

Add a new driver for the PIC32MX chip in FreeRTOS Plus TCP.

  • BufferAllocation_2.c
    • Include necessary headers for buffer allocation.
    • Implement buffer allocation functions for PIC32MX.
  • NetworkInterface_eth.c
    • Include necessary headers for Ethernet driver.
    • Implement Ethernet driver functions for PIC32MX.
  • CMakeLists.txt
    • Add CMake configuration for PIC32MX Ethernet driver.

Related to FreeRTOS#789

Add a new driver for the PIC32MX chip in FreeRTOS Plus TCP.

* **BufferAllocation_2.c**
  - Include necessary headers for buffer allocation.
  - Implement buffer allocation functions for PIC32MX.
* **NetworkInterface_eth.c**
  - Include necessary headers for Ethernet driver.
  - Implement Ethernet driver functions for PIC32MX.
* **CMakeLists.txt**
  - Add CMake configuration for PIC32MX Ethernet driver.
@vishwamartur vishwamartur requested a review from a team as a code owner November 2, 2024 04:49
Copy link
Member

@ActoryOu ActoryOu left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Left my comments here. Please also share your verification detail with us!

@@ -0,0 +1,634 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to not reuse the buffer allocation file at source/portable/BufferManagement?

Copy link
Contributor

@htibosch htibosch Nov 3, 2024

Choose a reason for hiding this comment

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

+1 for me
I don't remember the differences, but with some thinking it should be possible to merge the two files. Every "special version" of a module needs extra testing and maintenance.
Hein

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the feedback! There are no code changes in BufferAllocation_2.c—it has been revised once. Please let me know if further clarification is required. Otherwise, you can disregard this file.

Copy link
Member

@ActoryOu ActoryOu Nov 4, 2024

Choose a reason for hiding this comment

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

Could you simply use that file in source/portable/BufferManagement instead of creating a duplicattion in this PR?

@aggarg
Copy link
Member

aggarg commented Nov 4, 2024

@vishwamartur Please describe how you tested these changes.

@aggarg
Copy link
Member

aggarg commented Nov 7, 2024

@vishwamartur Please share the testing details. Would you also share the details about how you developed these changes.

@vishwamartur
Copy link
Author

@aggarg Thank you for your feedback!

At the moment, I have not been able to test these changes directly on a PIC32MX device. Unfortunately, I don't currently have access to the hardware needed for thorough testing. I'm considering options such as using PICSimLab, as it provides a simulated environment for PIC microcontrollers, though it may not fully replicate all aspects of the PIC32MX Ethernet functionality. Alternatively, I could put this PR on hold until I can complete testing with the actual device.

Let me know if you have any suggestions or if putting the PR on hold sounds reasonable.

@aggarg
Copy link
Member

aggarg commented Nov 10, 2024

Thank your for sharing! I am closing this PR for now. Please create a new one when you are done with the testing.

@aggarg aggarg closed this Nov 10, 2024
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